Skip to contentSkip to conversation

dm: fix freeze_bdev() return value check

PATCH In review Akinobu Mita 4 participants 5 messages Sep 24, 2007 – Sep 25, 2007 Device mapper

lore
Share & tools
lore

Akinobu Mita

author
#0 lore raw

PATCH 1/1 dm: fix freeze_bdev() return value check

freeze_bdev() does not return errno as pointer on failure. This patch fixes the return value check.

Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/md/dm.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
drivers/md/dm.c +2 -7
Index: 2.6-git/drivers/md/dm.c
===================================================================
@@ -1290,16 +1290,11 @@ out:
  */
 static int lock_fs(struct mapped_device *md)
 {
-	int r;
-
 	WARN_ON(md->frozen_sb);
 
 	md->frozen_sb = freeze_bdev(md->suspended_bdev);
-	if (IS_ERR(md->frozen_sb)) {
-		r = PTR_ERR(md->frozen_sb);
-		md->frozen_sb = NULL;
-		return r;
-	}
+	if (!md->frozen_sb)
+		return -ENODEV;
 
 	set_bit(DMF_FROZEN, &md->flags);
maintainer
#1 lore raw
Quoted 1 line · Akinobu Mita

freeze_bdev() does not return errno as pointer on failure.

Please check the history of this - was this ever in the tree or is it still pending or not?

Alasdair

Signature · 1 line
[email protected]
author
#2 lore raw
In reply to Akinobu Mita

2007/9/24, Akinobu Mita <[email protected]>:

Quoted 2 lines

freeze_bdev() does not return errno as pointer on failure. This patch fixes the return value check.

I misunderstood the meaning of the return value of freeze_bdev(). I thought NULL returned by freeze_bdev means error. But it is wrong. freeze_bdev returns NULL when there is no filesystem mounted on the device with holding bd_mount_sem.
So this patch is totally broken.

But there is no reason to check the return value with IS_ERR(). Because freeze_bdev return NULL or valid super block

Subject: [PATCH] dm: remove freeze_bdev() return value check
From: Akinobu Mita <[email protected]>

There is no reason to check the return value with IS_ERR(). Because freeze_bdev return NULL or valid super block.

Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/md/dm.c |    7 -------
 1 file changed, 7 deletions(-)
drivers/md/dm.c +0 -7
Index: 2.6-git/drivers/md/dm.c
===================================================================
@@ -1290,16 +1290,9 @@ out:
  */
 static int lock_fs(struct mapped_device *md)
 {
-	int r;
-
 	WARN_ON(md->frozen_sb);
 
 	md->frozen_sb = freeze_bdev(md->suspended_bdev);
-	if (IS_ERR(md->frozen_sb)) {
-		r = PTR_ERR(md->frozen_sb);
-		md->frozen_sb = NULL;
-		return r;
-	}
 
 	set_bit(DMF_FROZEN, &md->flags);
#3 lore raw
Quoted 4 lines · Akinobu Mita

2007/9/24, Akinobu Mita <[email protected]>:

Quoted 2 lines

freeze_bdev() does not return errno as pointer on failure. This patch fixes the return value check.

...

Quoted 2 lines

But there is no reason to check the return value with IS_ERR(). Because freeze_bdev return NULL or valid super block

Yes, but there is another place in kernel using this check (see /fs/xfs/xfs_fsops.c)

Probably question for Christoph Hellwig - some changes related to returning error in frezze_bdev() were planned but never commited. (see list archive)

Otherwise test for IS_ERR() should be removed.

Milan
--
[email protected]

Quoted 34 lines

Subject: [PATCH] dm: remove freeze_bdev() return value check
From: Akinobu Mita <[email protected]>

There is no reason to check the return value with IS_ERR(). Because freeze_bdev return NULL or valid super block.

Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/md/dm.c |    7 -------
 1 file changed, 7 deletions(-)

Index: 2.6-git/drivers/md/dm.c
===================================================================

drivers/md/dm.c +0 -7
@@ -1290,16 +1290,9 @@ out:
  */
 static int lock_fs(struct mapped_device *md)
 {
-	int r;
-
 	WARN_ON(md->frozen_sb);
 
 	md->frozen_sb = freeze_bdev(md->suspended_bdev);
-	if (IS_ERR(md->frozen_sb)) {
-		r = PTR_ERR(md->frozen_sb);
-		md->frozen_sb = NULL;
-		return r;
-	}
 
 	set_bit(DMF_FROZEN, &md->flags);
#4 lore raw
Quoted 9 lines · Milan Broz
Quoted 4 lines · Akinobu Mita

2007/9/24, Akinobu Mita <[email protected]>:

Quoted 2 lines

freeze_bdev() does not return errno as pointer on failure. This patch fixes the return value check.

...

Quoted 2 lines

But there is no reason to check the return value with IS_ERR(). Because freeze_bdev return NULL or valid super block

Quoted 8 lines

Yes, but there is another place in kernel using this check (see /fs/xfs/xfs_fsops.c)

Probably question for Christoph Hellwig - some changes related to returning error in frezze_bdev() were planned but never commited. (see list archive)

Otherwise test for IS_ERR() should be removed.

The plan was to change freeze_bdev to do a trylock un s_umount_sem and return SBUSY if it fails to avoid the deadlock scenario where people called xfs_freeze and got another freeze request from dm.

I don't remember why this never got it, it's probably worth sending a patch like this again.