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

[Vladimir Davydov]

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?

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