* [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio
@ 2020-02-25 17:07 ` Daniel Glöckner
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Glöckner @ 2020-02-25 17:07 UTC (permalink / raw)
To: Mike Snitzer, Mikulas Patocka; +Cc: dm-devel, linux-block, Daniel Glöckner
In cases where dec_in_flight has to requeue the integrity_bio_wait work
to transfer the rest of the data, the __bi_remaining field of the bio
might already have been decremented to zero. Reusing the bio without
reinitializing that counter to 1 can then result in integrity_end_io
being called too early when the BIO_CHAIN flag is set, f.ex. due to
blk_queue_split. In our case this triggered the BUG() in
blk_mq_end_request when the hardware signalled completion of the bio
after integrity_end_io had modified it.
Signed-off-by: Daniel Glöckner <dg@emlix.com>
---
drivers/md/dm-integrity.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index b225b3e445fa..8cea2978fc24 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -1438,6 +1438,7 @@ static void dec_in_flight(struct dm_integrity_io *dio)
if (likely(!bio->bi_status) && unlikely(bio_sectors(bio) != dio->range.n_sectors)) {
dio->range.logical_sector += dio->range.n_sectors;
bio_advance(bio, dio->range.n_sectors << SECTOR_SHIFT);
+ atomic_set(&bio->__bi_remaining, 1);
INIT_WORK(&dio->work, integrity_bio_wait);
queue_work(ic->wait_wq, &dio->work);
return;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio
@ 2020-02-25 17:07 ` Daniel Glöckner
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Glöckner @ 2020-02-25 17:07 UTC (permalink / raw)
To: Mike Snitzer, Mikulas Patocka; +Cc: linux-block, dm-devel, Daniel Glöckner
In cases where dec_in_flight has to requeue the integrity_bio_wait work
to transfer the rest of the data, the __bi_remaining field of the bio
might already have been decremented to zero. Reusing the bio without
reinitializing that counter to 1 can then result in integrity_end_io
being called too early when the BIO_CHAIN flag is set, f.ex. due to
blk_queue_split. In our case this triggered the BUG() in
blk_mq_end_request when the hardware signalled completion of the bio
after integrity_end_io had modified it.
Signed-off-by: Daniel Glöckner <dg@emlix.com>
---
drivers/md/dm-integrity.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index b225b3e445fa..8cea2978fc24 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -1438,6 +1438,7 @@ static void dec_in_flight(struct dm_integrity_io *dio)
if (likely(!bio->bi_status) && unlikely(bio_sectors(bio) != dio->range.n_sectors)) {
dio->range.logical_sector += dio->range.n_sectors;
bio_advance(bio, dio->range.n_sectors << SECTOR_SHIFT);
+ atomic_set(&bio->__bi_remaining, 1);
INIT_WORK(&dio->work, integrity_bio_wait);
queue_work(ic->wait_wq, &dio->work);
return;
--
2.17.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio
2020-02-25 17:07 ` Daniel Glöckner
(?)
@ 2020-02-25 19:12 ` Christoph Hellwig
2020-02-25 19:54 ` Daniel Glöckner
-1 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2020-02-25 19:12 UTC (permalink / raw)
To: Daniel Glöckner; +Cc: Mike Snitzer, Mikulas Patocka, linux-block, dm-devel
On Tue, Feb 25, 2020 at 06:07:44PM +0100, Daniel Glöckner wrote:
> In cases where dec_in_flight has to requeue the integrity_bio_wait work
> to transfer the rest of the data, the __bi_remaining field of the bio
> might already have been decremented to zero. Reusing the bio without
> reinitializing that counter to 1 can then result in integrity_end_io
> being called too early when the BIO_CHAIN flag is set, f.ex. due to
> blk_queue_split. In our case this triggered the BUG() in
> blk_mq_end_request when the hardware signalled completion of the bio
> after integrity_end_io had modified it.
>
> Signed-off-by: Daniel Glöckner <dg@emlix.com>
Drivers have no business poking into these internals. If a bio is
reused the caller needs to use bio_reset instead.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio
2020-02-25 19:12 ` [dm-devel] " Christoph Hellwig
@ 2020-02-25 19:54 ` Daniel Glöckner
2020-02-25 22:02 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Glöckner @ 2020-02-25 19:54 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Mike Snitzer, Mikulas Patocka, linux-block, dm-devel
Hello Christoph,
Am 02/25/20 um 20:12 schrieb Christoph Hellwig:
> On Tue, Feb 25, 2020 at 06:07:44PM +0100, Daniel Glöckner wrote:
>> In cases where dec_in_flight has to requeue the integrity_bio_wait work
>> to transfer the rest of the data, the __bi_remaining field of the bio
>> might already have been decremented to zero. Reusing the bio without
>> reinitializing that counter to 1 can then result in integrity_end_io
>> being called too early when the BIO_CHAIN flag is set, f.ex. due to
>> blk_queue_split. In our case this triggered the BUG() in
>> blk_mq_end_request when the hardware signalled completion of the bio
>> after integrity_end_io had modified it.
>>
>> Signed-off-by: Daniel Glöckner <dg@emlix.com>
>
> Drivers have no business poking into these internals. If a bio is
> reused the caller needs to use bio_reset instead.
bio_reset will reset too many fields. As you can see in the context of
the diff, dm-integrity expects f.ex. the values modified by bio_advance
to stay intact and the transfer should of course use the same disk and
operation.
How about doing the atomic_set in bio_remaining_done (in block/bio.c)
where the BIO_CHAIN flag is cleared once __bi_remaining hits zero?
Or is requeuing a bio without bio_reset really a no-go? In that case a
one-liner won't do...
Best regards,
Daniel
--
Besuchen Sie uns auf der Embedded World 2020 in Nürnberg!
-> Halle 4, Stand 368
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11,
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke
Ust-IdNr.: DE 205 198 055
emlix - your embedded linux partner
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio
2020-02-25 19:54 ` Daniel Glöckner
@ 2020-02-25 22:02 ` Christoph Hellwig
2020-02-26 1:22 ` Mike Snitzer
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2020-02-25 22:02 UTC (permalink / raw)
To: Daniel Glöckner
Cc: Christoph Hellwig, linux-block, dm-devel, Mikulas Patocka, Mike Snitzer
On Tue, Feb 25, 2020 at 08:54:07PM +0100, Daniel Glöckner wrote:
> bio_reset will reset too many fields. As you can see in the context of
> the diff, dm-integrity expects f.ex. the values modified by bio_advance
> to stay intact and the transfer should of course use the same disk and
> operation.
>
> How about doing the atomic_set in bio_remaining_done (in block/bio.c)
> where the BIO_CHAIN flag is cleared once __bi_remaining hits zero?
> Or is requeuing a bio without bio_reset really a no-go? In that case a
> one-liner won't do...
That tends to add a overhead to the fast path for a rather exotic
case. I'm having a bit of a hard time understanding the dm-integrity
code due to it's annoyingly obsfucated code, but it seems like it
tries to submit a bio again after it came out of a ->end_io handler.
That might have some other problems, but if we only want to paper
over the remaining count a isngle call to bio_inc_remaining might be all
you need.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dm integrity: reinitialize __bi_remaining when reusing bio
2020-02-25 22:02 ` Christoph Hellwig
@ 2020-02-26 1:22 ` Mike Snitzer
0 siblings, 0 replies; 6+ messages in thread
From: Mike Snitzer @ 2020-02-26 1:22 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Daniel Glöckner, linux-block, dm-devel, Mikulas Patocka
On Tue, Feb 25 2020 at 5:02pm -0500,
Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Feb 25, 2020 at 08:54:07PM +0100, Daniel Glöckner wrote:
> > bio_reset will reset too many fields. As you can see in the context of
> > the diff, dm-integrity expects f.ex. the values modified by bio_advance
> > to stay intact and the transfer should of course use the same disk and
> > operation.
> >
> > How about doing the atomic_set in bio_remaining_done (in block/bio.c)
> > where the BIO_CHAIN flag is cleared once __bi_remaining hits zero?
> > Or is requeuing a bio without bio_reset really a no-go? In that case a
> > one-liner won't do...
>
> That tends to add a overhead to the fast path for a rather exotic
> case. I'm having a bit of a hard time understanding the dm-integrity
> code due to it's annoyingly obsfucated code, but it seems like it
> tries to submit a bio again after it came out of a ->end_io handler.
Yeah, the dm-integrity code has always been complex and it has only
gotten moreso. I'm struggling with it too.
This case that Daniel has seen a BUG_ON with is when there is a need to
finish a partially completed bio (as reflected by the per-bio-data's
internal accounting managed by dm-integrity).
> That might have some other problems, but if we only want to paper
> over the remaining count a isngle call to bio_inc_remaining might be all
> you need.
bio_inc_remaining() is really meant to be paired with bio_chain(). They
are pretty tightly coupled these days. We removed __bio_inc_remaining()
once we weened all (ab)users many releases ago. Definitely don't want
an open-coded equivalent buried in an obscure dm-integrity usecase.
Could be bio_split() + generic_make_request() recursion is a way
forward -- but that'd likely require some extra work in dm-integrity.
All the additional code in dm_integrity_map_continue() makes me think I
could easily be missing something.
Mikulas, if you could look closer at this issue and see what your best
suggestion would be that'd be appreciated.
Thanks,
Mike
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-26 1:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25 17:07 [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio Daniel Glöckner
2020-02-25 17:07 ` Daniel Glöckner
2020-02-25 19:12 ` [dm-devel] " Christoph Hellwig
2020-02-25 19:54 ` Daniel Glöckner
2020-02-25 22:02 ` Christoph Hellwig
2020-02-26 1:22 ` Mike Snitzer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.