| Message ID | 20171208230235.30636-1-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(Add Jérôme Carretero.) Thanks, -liubo On Fri, Dec 08, 2017 at 04:02:35PM -0700, Liu Bo wrote: > We're not allowed to take any new bios to rbio->bio_list in > rbio_orig_end_io(), otherwise we may get merged with more bios and > rbio->bio_list is not empty. > > This should only happens in error-out cases, the normal path of > recover and full stripe write have already set RBIO_RMW_LOCKED_BIT to > disable merge before doing IO. > > Reported-by: Jérôme Carretero <cJ-ko@zougloub.eu> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/raid56.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > index 5aa9d22..127c782 100644 > --- a/fs/btrfs/raid56.c > +++ b/fs/btrfs/raid56.c > @@ -859,12 +859,23 @@ static void free_raid_bio(struct btrfs_raid_bio *rbio) > */ > static void rbio_orig_end_io(struct btrfs_raid_bio *rbio, blk_status_t err) > { > - struct bio *cur = bio_list_get(&rbio->bio_list); > + struct bio *cur; > struct bio *next; > > + /* > + * We're not allowed to take any new bios to rbio->bio_list > + * from now on, otherwise we may get merged with more bios and > + * rbio->bio_list is not empty. > + */ > + spin_lock(&rbio->bio_list_lock); > + set_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags); > + spin_unlock(&rbio->bio_list_lock); > + > if (rbio->generic_bio_cnt) > btrfs_bio_counter_sub(rbio->fs_info, rbio->generic_bio_cnt); > > + cur = bio_list_get(&rbio->bio_list); > + > free_raid_bio(rbio); > > while (cur) { > -- > 2.9.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9.12.2017 01:02, Liu Bo wrote: > We're not allowed to take any new bios to rbio->bio_list in > rbio_orig_end_io(), otherwise we may get merged with more bios and > rbio->bio_list is not empty. > > This should only happens in error-out cases, the normal path of > recover and full stripe write have already set RBIO_RMW_LOCKED_BIT to > disable merge before doing IO. > > Reported-by: Jérôme Carretero <cJ-ko@zougloub.eu> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/raid56.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > index 5aa9d22..127c782 100644 > --- a/fs/btrfs/raid56.c > +++ b/fs/btrfs/raid56.c > @@ -859,12 +859,23 @@ static void free_raid_bio(struct btrfs_raid_bio *rbio) > */ > static void rbio_orig_end_io(struct btrfs_raid_bio *rbio, blk_status_t err) > { > - struct bio *cur = bio_list_get(&rbio->bio_list); > + struct bio *cur; > struct bio *next; > > + /* > + * We're not allowed to take any new bios to rbio->bio_list > + * from now on, otherwise we may get merged with more bios and > + * rbio->bio_list is not empty. > + */ > + spin_lock(&rbio->bio_list_lock); > + set_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags); > + spin_unlock(&rbio->bio_list_lock); do we really need the spinlock, bit operations are atomic? > + > if (rbio->generic_bio_cnt) > btrfs_bio_counter_sub(rbio->fs_info, rbio->generic_bio_cnt); > > + cur = bio_list_get(&rbio->bio_list); > + > free_raid_bio(rbio); > > while (cur) { > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 09, 2017 at 03:32:18PM +0200, Nikolay Borisov wrote: > > > On 9.12.2017 01:02, Liu Bo wrote: > > We're not allowed to take any new bios to rbio->bio_list in > > rbio_orig_end_io(), otherwise we may get merged with more bios and > > rbio->bio_list is not empty. > > > > This should only happens in error-out cases, the normal path of > > recover and full stripe write have already set RBIO_RMW_LOCKED_BIT to > > disable merge before doing IO. > > > > Reported-by: Jérôme Carretero <cJ-ko@zougloub.eu> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > fs/btrfs/raid56.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > > index 5aa9d22..127c782 100644 > > --- a/fs/btrfs/raid56.c > > +++ b/fs/btrfs/raid56.c > > @@ -859,12 +859,23 @@ static void free_raid_bio(struct btrfs_raid_bio *rbio) > > */ > > static void rbio_orig_end_io(struct btrfs_raid_bio *rbio, blk_status_t err) > > { > > - struct bio *cur = bio_list_get(&rbio->bio_list); > > + struct bio *cur; > > struct bio *next; > > > > + /* > > + * We're not allowed to take any new bios to rbio->bio_list > > + * from now on, otherwise we may get merged with more bios and > > + * rbio->bio_list is not empty. > > + */ > > + spin_lock(&rbio->bio_list_lock); > > + set_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags); > > + spin_unlock(&rbio->bio_list_lock); > > do we really need the spinlock, bit operations are atomic? > Thanks for the question. Atomicity doesn't really matter here, set_bit() needs to be done in the critical section so that merge_rbio() can do right things if merge_rbio() comes after rbio_orig_end_io(). thanks, -liubo > > + > > if (rbio->generic_bio_cnt) > > btrfs_bio_counter_sub(rbio->fs_info, rbio->generic_bio_cnt); > > > > + cur = bio_list_get(&rbio->bio_list); > > + > > free_raid_bio(rbio); > > > > while (cur) { > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 08, 2017 at 04:02:35PM -0700, Liu Bo wrote: > We're not allowed to take any new bios to rbio->bio_list in > rbio_orig_end_io(), otherwise we may get merged with more bios and > rbio->bio_list is not empty. > > This should only happens in error-out cases, the normal path of > recover and full stripe write have already set RBIO_RMW_LOCKED_BIT to > disable merge before doing IO. > > Reported-by: Jérôme Carretero <cJ-ko@zougloub.eu> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> Added to next, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 5aa9d22..127c782 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -859,12 +859,23 @@ static void free_raid_bio(struct btrfs_raid_bio *rbio) */ static void rbio_orig_end_io(struct btrfs_raid_bio *rbio, blk_status_t err) { - struct bio *cur = bio_list_get(&rbio->bio_list); + struct bio *cur; struct bio *next; + /* + * We're not allowed to take any new bios to rbio->bio_list + * from now on, otherwise we may get merged with more bios and + * rbio->bio_list is not empty. + */ + spin_lock(&rbio->bio_list_lock); + set_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags); + spin_unlock(&rbio->bio_list_lock); + if (rbio->generic_bio_cnt) btrfs_bio_counter_sub(rbio->fs_info, rbio->generic_bio_cnt); + cur = bio_list_get(&rbio->bio_list); + free_raid_bio(rbio); while (cur) {
We're not allowed to take any new bios to rbio->bio_list in rbio_orig_end_io(), otherwise we may get merged with more bios and rbio->bio_list is not empty. This should only happens in error-out cases, the normal path of recover and full stripe write have already set RBIO_RMW_LOCKED_BIT to disable merge before doing IO. Reported-by: Jérôme Carretero <cJ-ko@zougloub.eu> Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/raid56.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)