[Devel] [PATCH rh7 1/4] fs: do not fail on double freeze bdev w/o sb

[Maxim Patlasov]

On 07/13/2016 02:46 AM, Vladimir Davydov wrote:

> On Tue, Jul 12, 2016 at 03:02:11PM -0700, Maxim Patlasov wrote:
>> Let's keep flies and cutlets separately. It seems we can easily satisfy
>> push-backup needs by implementing freeze/thaw ploop ioctls without tackling
>> generic code at all, see a patch in attachment (unless I missed something
>> obvious). And apart from these ploop/push-backup stuff, if you think your
>> changes for freeze_bdev() and thaw_bdev() are useful, send them upstream, so
>> we'll back-port them later, when they are accepted upstream (unless I missed
>> some scenario for which those changes matter for us). In the other words, I
>> think we have to keep our vz7 generic code base closer to ms, unless we have
>> good reason to deviate.
> Agree. Generally, I like your patch more than mine, but I've a concern
> about it - see below.
>
>> On 07/12/2016 03:04 AM, Vladimir Davydov wrote:
>>> It's possible to freeze a bdev which is not mounted. In this case
>>> freeze_bdev() only increments bd_fsfrozen_count in order to prevent the
>>> bdev from being mounted and does nothing else. A second freeze attempt
>>> on the same device is supposed to increment bd_fsfrozen_count again, but
>>> it results in NULL ptr dereference, because freeze_bdev() doesn't check
>>> the return value of get_super(). Fix that.
>>>
>>> Signed-off-by: Vladimir Davydov <vdavydov at virtuozzo.com>
>>> ---
>>>   fs/block_dev.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>>> index 4575c62d8b0b..325ee7161fbf 100644
>>> --- a/fs/block_dev.c
>>> +++ b/fs/block_dev.c
>>> @@ -227,7 +227,8 @@ struct super_block *freeze_bdev(struct block_device *bdev)
>>>   		 * thaw_bdev drops it.
>>>   		 */
>>>   		sb = get_super(bdev);
>>> -		drop_super(sb);
>>> +		if (sb)
>>> +			drop_super(sb);
>>>   		mutex_unlock(&bdev->bd_fsfreeze_mutex);
>>>   		return sb;
>>>   	}
>> The ioctls simply freeze and thaw ploop bdev.
>>
>> Caveats:
>>
>> 1) If no fs mounted, the ioctls have no effect.
>> 2) No nested freeze: many PLOOP_IOC_FREEZE ioctls have the same effect as one.
>> 3) The same for thaw.
> I think #2 and #3 are OK. But regarding #1 - what if we want to make a
> backup of a secondary ploop which is not mounted? So we try to freeze it
> and succeed, but it isn't actually frozen, so it can be mounted and
> modified while we're backing it up, which is incorrect AFAIU.
>
> What about something like this on top of your patch?

You're right. The patch looks correct and it works for me. You can add
Acked-by: Maxim Patlasov <mpatlasov at virtuozzo.com>

>
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 9a9cc8b0b934..d52975eaaa36 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -4819,16 +4819,15 @@ static int ploop_freeze(struct ploop_device *plo, struct block_device *bdev)
>   {
>   	struct super_block *sb = plo->sb;
>   
> -	if (sb)
> +	if (test_bit(PLOOP_S_FROZEN, &plo->state))
>   		return 0;
>   
>   	sb = freeze_bdev(bdev);
>   	if (sb && IS_ERR(sb))
>   		return PTR_ERR(sb);
> -	if (!sb)
> -		thaw_bdev(bdev, sb);
>   
>   	plo->sb = sb;
> +	set_bit(PLOOP_S_FROZEN, &plo->state);
>   	return 0;
>   }
>   
> @@ -4837,12 +4836,14 @@ static int ploop_thaw(struct ploop_device *plo, struct block_device *bdev)
>   	struct super_block *sb = plo->sb;
>   	int err;
>   
> -	if (!sb)
> +	if (!test_bit(PLOOP_S_FROZEN, &plo->state))
>   		return 0;
>   
>   	err = thaw_bdev(bdev, sb);
> -	if (!err)
> +	if (!err) {
>   		plo->sb = NULL;
> +		clear_bit(PLOOP_S_FROZEN, &plo->state);
> +	}
>   
>   	return err;
>   }
> diff --git a/include/linux/ploop/ploop.h b/include/linux/ploop/ploop.h
> index 6ae96c4486fe..7864edf17f19 100644
> --- a/include/linux/ploop/ploop.h
> +++ b/include/linux/ploop/ploop.h
> @@ -61,6 +61,7 @@ enum {
>   				   (for minor mgmt only) */
>   	PLOOP_S_ONCE,	        /* An event (e.g. printk once) happened */
>   	PLOOP_S_PUSH_BACKUP,	/* Push_backup is in progress */
> +	PLOOP_S_FROZEN		/* Frozen PLOOP_IOC_FREEZE */
>   };
>   
>   struct ploop_snapdata