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 <akinobu.mitagmail.com>

---
 drivers/md/dm.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Index: 2.6-git/drivers/md/dm.c
============================================================
=======
--- 2.6-git.orig/drivers/md/dm.c
+++ 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);
 

--
dm-devel mailing list
dm-develredhat.com
http
s://www.redhat.com/mailman/listinfo/dm-devel

On Mon, Sep 24, 2007 at 07:56:36PM +0900, Akinobu Mita
wrote:
> 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
-- 
agkredhat.com

--
dm-devel mailing list
dm-develredhat.com
http
s://www.redhat.com/mailman/listinfo/dm-devel

2007/9/24, Akinobu Mita <akinobu.mitagmail.com>:
> 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 <akinobu.mitagmail.com>

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 <akinobu.mitagmail.com>

---
 drivers/md/dm.c |    7 -------
 1 file changed, 7 deletions(-)

Index: 2.6-git/drivers/md/dm.c
============================================================
=======
--- 2.6-git.orig/drivers/md/dm.c
+++ 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);

--
dm-devel mailing list
dm-develredhat.com
http
s://www.redhat.com/mailman/listinfo/dm-devel

Akinobu Mita wrote:
> 2007/9/24, Akinobu Mita <akinobu.mitagmail.com>:
>> freeze_bdev() does not return errno as pointer on
failure.
>> This patch fixes the return value check.
> 
...
> 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
--
mbrozredhat.com

> 
> Subject: [PATCH] dm: remove freeze_bdev() return value
check
> From: Akinobu Mita <akinobu.mitagmail.com>
> 
> 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 <akinobu.mitagmail.com>
> 
> ---
>  drivers/md/dm.c |    7 -------
>  1 file changed, 7 deletions(-)
> 
> Index: 2.6-git/drivers/md/dm.c
>
============================================================
=======
> --- 2.6-git.orig/drivers/md/dm.c
> +++ 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);
> 

--
dm-devel mailing list
dm-develredhat.com
http
s://www.redhat.com/mailman/listinfo/dm-devel

On Tue, Sep 25, 2007 at 05:52:29PM +0200, Milan Broz wrote:
> Akinobu Mita wrote:
> > 2007/9/24, Akinobu Mita <akinobu.mitagmail.com>:
> >> freeze_bdev() does not return errno as pointer
on failure.
> >> This patch fixes the return value check.
> > 
> ...
> > 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.

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.

--
dm-devel mailing list
dm-develredhat.com
http
s://www.redhat.com/mailman/listinfo/dm-devel