* Re: [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io
2017-12-08 23:02 [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io Liu Bo
@ 2017-12-08 23:05 ` Liu Bo
2017-12-09 13:32 ` Nikolay Borisov
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Liu Bo @ 2017-12-08 23:05 UTC (permalink / raw)
To: linux-btrfs; +Cc: Jérôme Carretero
(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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io
2017-12-08 23:02 [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io Liu Bo
2017-12-08 23:05 ` Liu Bo
@ 2017-12-09 13:32 ` Nikolay Borisov
2017-12-11 20:09 ` Liu Bo
2017-12-12 19:15 ` David Sterba
2018-01-10 1:36 ` [PATCH v2] " Liu Bo
3 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2017-12-09 13:32 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
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) {
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io
2017-12-09 13:32 ` Nikolay Borisov
@ 2017-12-11 20:09 ` Liu Bo
0 siblings, 0 replies; 6+ messages in thread
From: Liu Bo @ 2017-12-11 20:09 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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) {
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io
2017-12-08 23:02 [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io Liu Bo
2017-12-08 23:05 ` Liu Bo
2017-12-09 13:32 ` Nikolay Borisov
@ 2017-12-12 19:15 ` David Sterba
2018-01-10 1:36 ` [PATCH v2] " Liu Bo
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2017-12-12 19:15 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io
2017-12-08 23:02 [PATCH] Btrfs: raid56: fix race between merge_bio and rbio_orig_end_io Liu Bo
` (2 preceding siblings ...)
2017-12-12 19:15 ` David Sterba
@ 2018-01-10 1:36 ` Liu Bo
3 siblings, 0 replies; 6+ messages in thread
From: Liu Bo @ 2018-01-10 1:36 UTC (permalink / raw)
To: linux-btrfs
Before rbio_orig_end_io() goes to free rbio, rbio may get merged with
more bios from other rbios and rbio->bio_list becomes non-empty,
in that case, these newly merged bios don't end properly.
Once unlock_stripe() is done, rbio->bio_list will not be updated any
more and we can call bio_endio() on all queued bios.
It should only happen 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, so rbio_orig_end_io() called by them doesn't
have the above issue.
Reported-by: Jérôme Carretero <cJ-ko@zougloub.eu>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v2: - Remove the usage spin_lock as there is a chance of deadlock in
interrupt context, it's reported by lockdep, although it'd never
happen because we've taken care of it by saving irq flags at all
places.
- Update commit log and comments of code to explain the new idea.
- This has been tested against btrfs/011 for 50 times.
fs/btrfs/raid56.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 7747323..b2b426d 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -864,10 +864,17 @@ static void __free_raid_bio(struct btrfs_raid_bio *rbio)
kfree(rbio);
}
-static void free_raid_bio(struct btrfs_raid_bio *rbio)
+static void rbio_endio_bio_list(struct bio *cur, blk_status_t err)
{
- unlock_stripe(rbio);
- __free_raid_bio(rbio);
+ struct bio *next;
+
+ while (cur) {
+ next = cur->bi_next;
+ cur->bi_next = NULL;
+ cur->bi_status = err;
+ bio_endio(cur);
+ cur = next;
+ }
}
/*
@@ -877,20 +884,26 @@ 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 *next;
+ struct bio *extra;
if (rbio->generic_bio_cnt)
btrfs_bio_counter_sub(rbio->fs_info, rbio->generic_bio_cnt);
- free_raid_bio(rbio);
+ /*
+ * At this moment, rbio->bio_list is empty, however since rbio does not
+ * always have RBIO_RMW_LOCKED_BIT set and rbio is still linked on the
+ * hash list, rbio may be merged with others so that rbio->bio_list
+ * becomes non-empty.
+ * Once unlock_stripe() is done, rbio->bio_list will not be updated any
+ * more and we can call bio_endio() on all queued bios.
+ */
+ unlock_stripe(rbio);
+ extra = bio_list_get(&rbio->bio_list);
+ __free_raid_bio(rbio);
- while (cur) {
- next = cur->bi_next;
- cur->bi_next = NULL;
- cur->bi_status = err;
- bio_endio(cur);
- cur = next;
- }
+ rbio_endio_bio_list(cur, err);
+ if (extra)
+ rbio_endio_bio_list(extra, err);
}
/*
--
2.9.4
^ permalink raw reply related [flat|nested] 6+ messages in thread