* [PATCH 0/2] loop: fix two issues introduced by prior commit
@ 2019-02-21 4:17 Dongli Zhang
2019-02-21 4:17 ` [PATCH 1/2] loop: do not print warn message if partition scan is successful Dongli Zhang
2019-02-21 4:17 ` [PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part() Dongli Zhang
0 siblings, 2 replies; 6+ messages in thread
From: Dongli Zhang @ 2019-02-21 4:17 UTC (permalink / raw)
To: linux-block, linux-kernel; +Cc: axboe, jack
This patch set fix two issues introduced by prior commit.
[PATCH 1/2] loop: do not print warn message if partition scan is successful
[PATCH 1/2] fixes d57f3374ba48 ("loop: Move special partition reread
handling in loop_clr_fd()") to not always print warn message even when
partition scan is successful.
[PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part()
[PATCH 2/2] fixes 0da03cab87e6 ("loop: Fix deadlock when calling
blkdev_reread_part()") to not set GENHD_FL_NO_PART_SCAN before partition
scan when detaching loop device from the file.
Thank you very much!
Dongli Zhang
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] loop: do not print warn message if partition scan is successful 2019-02-21 4:17 [PATCH 0/2] loop: fix two issues introduced by prior commit Dongli Zhang @ 2019-02-21 4:17 ` Dongli Zhang 2019-02-21 11:01 ` Jan Kara 2019-02-21 4:17 ` [PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part() Dongli Zhang 1 sibling, 1 reply; 6+ messages in thread From: Dongli Zhang @ 2019-02-21 4:17 UTC (permalink / raw) To: linux-block, linux-kernel; +Cc: axboe, jack Do not print warn message when the partition scan returns 0. Fixes: d57f3374ba48 ("loop: Move special partition reread handling in loop_clr_fd()") Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- drivers/block/loop.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index cf55389..7908673 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1115,8 +1115,9 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) err = __blkdev_reread_part(bdev); else err = blkdev_reread_part(bdev); - pr_warn("%s: partition scan of loop%d failed (rc=%d)\n", - __func__, lo_number, err); + if (err) + pr_warn("%s: partition scan of loop%d failed (rc=%d)\n", + __func__, lo_number, err); /* Device is gone, no point in returning error */ err = 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] loop: do not print warn message if partition scan is successful 2019-02-21 4:17 ` [PATCH 1/2] loop: do not print warn message if partition scan is successful Dongli Zhang @ 2019-02-21 11:01 ` Jan Kara 0 siblings, 0 replies; 6+ messages in thread From: Jan Kara @ 2019-02-21 11:01 UTC (permalink / raw) To: Dongli Zhang; +Cc: linux-block, linux-kernel, axboe, jack On Thu 21-02-19 12:17:34, Dongli Zhang wrote: > Do not print warn message when the partition scan returns 0. > > Fixes: d57f3374ba48 ("loop: Move special partition reread handling in loop_clr_fd()") > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> Yeah, thanks for the fix! Not sure how come I didn't see this in my testing. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > drivers/block/loop.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index cf55389..7908673 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1115,8 +1115,9 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) > err = __blkdev_reread_part(bdev); > else > err = blkdev_reread_part(bdev); > - pr_warn("%s: partition scan of loop%d failed (rc=%d)\n", > - __func__, lo_number, err); > + if (err) > + pr_warn("%s: partition scan of loop%d failed (rc=%d)\n", > + __func__, lo_number, err); > /* Device is gone, no point in returning error */ > err = 0; > } > -- > 2.7.4 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part() 2019-02-21 4:17 [PATCH 0/2] loop: fix two issues introduced by prior commit Dongli Zhang 2019-02-21 4:17 ` [PATCH 1/2] loop: do not print warn message if partition scan is successful Dongli Zhang @ 2019-02-21 4:17 ` Dongli Zhang 2019-02-21 11:30 ` Jan Kara 1 sibling, 1 reply; 6+ messages in thread From: Dongli Zhang @ 2019-02-21 4:17 UTC (permalink / raw) To: linux-block, linux-kernel; +Cc: axboe, jack Commit 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") moves blkdev_reread_part() out of the loop_ctl_mutex. However, GENHD_FL_NO_PART_SCAN is set before __blkdev_reread_part(). As a result, __blkdev_reread_part() will fail the check of GENHD_FL_NO_PART_SCAN and will not rescan the loop device to delete all partitions. Below are steps to reproduce the issue: step1 # dd if=/dev/zero of=tmp.raw bs=1M count=100 step2 # losetup -P /dev/loop0 tmp.raw step3 # parted /dev/loop0 mklabel gpt step4 # parted -a none -s /dev/loop0 mkpart primary 64s 1 step5 # losetup -d /dev/loop0 Step5 will not be able to delete /dev/loop0p1 (introduced by step4) and there is below kernel warning message: [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) This patch sets GENHD_FL_NO_PART_SCAN after blkdev_reread_part(). Fixes: 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- drivers/block/loop.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 7908673..736e55b 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1034,6 +1034,15 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer, return err; } +static void loop_disable_partscan(struct loop_device *lo) +{ + mutex_lock(&loop_ctl_mutex); + lo->lo_flags = 0; + if (!part_shift) + lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; + mutex_unlock(&loop_ctl_mutex); +} + static int __loop_clr_fd(struct loop_device *lo, bool release) { struct file *filp = NULL; @@ -1096,9 +1105,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) partscan = lo->lo_flags & LO_FLAGS_PARTSCAN && bdev; lo_number = lo->lo_number; - lo->lo_flags = 0; - if (!part_shift) - lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; loop_unprepare_queue(lo); out_unlock: mutex_unlock(&loop_ctl_mutex); @@ -1121,6 +1127,9 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) /* Device is gone, no point in returning error */ err = 0; } + + loop_disable_partscan(lo); + /* * Need not hold loop_ctl_mutex to fput backing file. * Calling fput holding loop_ctl_mutex triggers a circular -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part() 2019-02-21 4:17 ` [PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part() Dongli Zhang @ 2019-02-21 11:30 ` Jan Kara 2019-02-21 15:25 ` Dongli Zhang 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2019-02-21 11:30 UTC (permalink / raw) To: Dongli Zhang; +Cc: linux-block, linux-kernel, axboe, jack On Thu 21-02-19 12:17:35, Dongli Zhang wrote: > Commit 0da03cab87e6 > ("loop: Fix deadlock when calling blkdev_reread_part()") moves > blkdev_reread_part() out of the loop_ctl_mutex. However, > GENHD_FL_NO_PART_SCAN is set before __blkdev_reread_part(). As a result, > __blkdev_reread_part() will fail the check of GENHD_FL_NO_PART_SCAN and > will not rescan the loop device to delete all partitions. > > Below are steps to reproduce the issue: > > step1 # dd if=/dev/zero of=tmp.raw bs=1M count=100 > step2 # losetup -P /dev/loop0 tmp.raw > step3 # parted /dev/loop0 mklabel gpt > step4 # parted -a none -s /dev/loop0 mkpart primary 64s 1 > step5 # losetup -d /dev/loop0 Can you perhaps write a blktest for this? Thanks! > Step5 will not be able to delete /dev/loop0p1 (introduced by step4) and > there is below kernel warning message: > > [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) > > This patch sets GENHD_FL_NO_PART_SCAN after blkdev_reread_part(). > > Fixes: 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > --- > drivers/block/loop.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 7908673..736e55b 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1034,6 +1034,15 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer, > return err; > } > > +static void loop_disable_partscan(struct loop_device *lo) > +{ > + mutex_lock(&loop_ctl_mutex); > + lo->lo_flags = 0; > + if (!part_shift) > + lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; > + mutex_unlock(&loop_ctl_mutex); > +} > + > static int __loop_clr_fd(struct loop_device *lo, bool release) > { > struct file *filp = NULL; > @@ -1096,9 +1105,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) > > partscan = lo->lo_flags & LO_FLAGS_PARTSCAN && bdev; > lo_number = lo->lo_number; > - lo->lo_flags = 0; > - if (!part_shift) > - lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; > loop_unprepare_queue(lo); > out_unlock: > mutex_unlock(&loop_ctl_mutex); > @@ -1121,6 +1127,9 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) > /* Device is gone, no point in returning error */ > err = 0; > } > + > + loop_disable_partscan(lo); > + > /* > * Need not hold loop_ctl_mutex to fput backing file. > * Calling fput holding loop_ctl_mutex triggers a circular So I don't think this change is actually correct. The problem is that once lo->lo_state is set to Lo_unbound and loop_ctl_mutex is unlocked, the loop device structure can be reused for a new device (bound to a new file). So you cannot safely manipulate flags on lo->lo_disk anymore. But I think we can just move the setting of lo->lo_state to Lo_unbound after partscan has finished as well. There cannot be anybody else entering __loop_clr_fd() as lo->lo_backing_file is already cleared and Lo_rundown state protects us from all the other places trying to change the 'lo' device (please make this last sentence into a comment in the code explaining why setting lo->lo_state so late is fine). Thanks! Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part() 2019-02-21 11:30 ` Jan Kara @ 2019-02-21 15:25 ` Dongli Zhang 0 siblings, 0 replies; 6+ messages in thread From: Dongli Zhang @ 2019-02-21 15:25 UTC (permalink / raw) To: Jan Kara; +Cc: linux-block, linux-kernel, axboe On 02/21/2019 07:30 PM, Jan Kara wrote: > On Thu 21-02-19 12:17:35, Dongli Zhang wrote: >> Commit 0da03cab87e6 >> ("loop: Fix deadlock when calling blkdev_reread_part()") moves >> blkdev_reread_part() out of the loop_ctl_mutex. However, >> GENHD_FL_NO_PART_SCAN is set before __blkdev_reread_part(). As a result, >> __blkdev_reread_part() will fail the check of GENHD_FL_NO_PART_SCAN and >> will not rescan the loop device to delete all partitions. >> >> Below are steps to reproduce the issue: >> >> step1 # dd if=/dev/zero of=tmp.raw bs=1M count=100 >> step2 # losetup -P /dev/loop0 tmp.raw >> step3 # parted /dev/loop0 mklabel gpt >> step4 # parted -a none -s /dev/loop0 mkpart primary 64s 1 >> step5 # losetup -d /dev/loop0 > > Can you perhaps write a blktest for this? Thanks! I will write a blktest for above case. Thanks for the suggestion. > >> Step5 will not be able to delete /dev/loop0p1 (introduced by step4) and >> there is below kernel warning message: >> >> [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) >> >> This patch sets GENHD_FL_NO_PART_SCAN after blkdev_reread_part(). >> >> Fixes: 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >> --- >> drivers/block/loop.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c >> index 7908673..736e55b 100644 >> --- a/drivers/block/loop.c >> +++ b/drivers/block/loop.c >> @@ -1034,6 +1034,15 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer, >> return err; >> } >> >> +static void loop_disable_partscan(struct loop_device *lo) >> +{ >> + mutex_lock(&loop_ctl_mutex); >> + lo->lo_flags = 0; >> + if (!part_shift) >> + lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; >> + mutex_unlock(&loop_ctl_mutex); >> +} >> + >> static int __loop_clr_fd(struct loop_device *lo, bool release) >> { >> struct file *filp = NULL; >> @@ -1096,9 +1105,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) >> >> partscan = lo->lo_flags & LO_FLAGS_PARTSCAN && bdev; >> lo_number = lo->lo_number; >> - lo->lo_flags = 0; >> - if (!part_shift) >> - lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; >> loop_unprepare_queue(lo); >> out_unlock: >> mutex_unlock(&loop_ctl_mutex); >> @@ -1121,6 +1127,9 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) >> /* Device is gone, no point in returning error */ >> err = 0; >> } >> + >> + loop_disable_partscan(lo); >> + >> /* >> * Need not hold loop_ctl_mutex to fput backing file. >> * Calling fput holding loop_ctl_mutex triggers a circular > > So I don't think this change is actually correct. The problem is that once > lo->lo_state is set to Lo_unbound and loop_ctl_mutex is unlocked, the loop > device structure can be reused for a new device (bound to a new file). So > you cannot safely manipulate flags on lo->lo_disk anymore. But I think we > can just move the setting of lo->lo_state to Lo_unbound after partscan has > finished as well. There cannot be anybody else entering __loop_clr_fd() as > lo->lo_backing_file is already cleared and Lo_rundown state protects us > from all the other places trying to change the 'lo' device (please make > this last sentence into a comment in the code explaining why setting > lo->lo_state so late is fine). Thanks! I will set lo->lo_state to Lo_unbound after partscan in v2. Thank you very much! Dongli Zhang > > Honza > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-21 15:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-21 4:17 [PATCH 0/2] loop: fix two issues introduced by prior commit Dongli Zhang 2019-02-21 4:17 ` [PATCH 1/2] loop: do not print warn message if partition scan is successful Dongli Zhang 2019-02-21 11:01 ` Jan Kara 2019-02-21 4:17 ` [PATCH 2/2] loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part() Dongli Zhang 2019-02-21 11:30 ` Jan Kara 2019-02-21 15:25 ` Dongli Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).