From: Ming Lei <ming.lei@redhat.com> To: Kent Overstreet <kent.overstreet@gmail.com> Cc: Jens Axboe <axboe@kernel.dk>, Mike Snitzer <snitzer@redhat.com>, linux-block@vger.kernel.org, dm-devel@redhat.com, Eric Biggers <ebiggers@google.com>, Dmitry Monakhov <dmonakhov@openvz.org>, "Martin K . Petersen" <martin.petersen@oracle.com> Subject: Re: [PATCH 5.20 1/4] block: add bio_rewind() API Date: Wed, 29 Jun 2022 09:02:58 +0800 [thread overview] Message-ID: <YrukwsgbFxd69wA1@T590> (raw) In-Reply-To: <20220628163617.h3bmq3opd7yuiaop@moria.home.lan> On Tue, Jun 28, 2022 at 12:36:17PM -0400, Kent Overstreet wrote: > On Tue, Jun 28, 2022 at 03:49:28PM +0800, Ming Lei wrote: > > On Tue, Jun 28, 2022 at 12:26:10AM -0400, Kent Overstreet wrote: > > > On Mon, Jun 27, 2022 at 03:36:22PM +0800, Ming Lei wrote: > > > > Not mention bio_iter, bvec_iter has been 32 bytes, which is too big to > > > > hold in per-io data structure. With this patch, 8bytes is enough > > > > to rewind one bio if the end sector is fixed. > > > > > > And with rewind, you're making an assumption about the state the iterator is > > > going to be in when the IO has completed. > > > > > > What if the iterator was never advanced? > > > > bio_rewind() works as expected if the iterator doesn't advance, since bytes > > between the recorded position and the end position isn't changed, same > > with the end position. > > > > > > > > So say you check for that by saving some other part of the iterator - but that > > > may have legitimately changed too, if the bio was redirected (bi_sector changes) > > > or trimmed (bi_size changes) > > > > > > I still think this is an inherently buggy interface, the way it's being proposed > > > to be used. > > > > The patch did mention that the interface should be for situation in which end > > sector of bio won't change. > > But that's an assumption that you simply can't make! Of course, we can, at least for this DM's use case, the bio is issued from FS or split from DM, and it won't be issued to underlying queue any more, and simply owned by DM core code. > > We allow block device drivers to be stacked in _any_ combination. After a bio is > completed it may have been partially advanced, fully advanced, trimmed, not > trimmed, anything - and bi_sector and thus also bio_end_sector() may have > changed, and will have if there's partition tables involved. How can bio (partial)advance change bio's end sector? bio end sector can be changed only when bio->bi_iter.bi_size is changed manually(include bio_trim), or ->bi_bdev is changed. But inside one driver, if the bio is owned by this driver(such as the driver is the finally layer request based driver), the assumption often can make. Thanks, Ming
WARNING: multiple messages have this Message-ID (diff)
From: Ming Lei <ming.lei@redhat.com> To: Kent Overstreet <kent.overstreet@gmail.com> Cc: Jens Axboe <axboe@kernel.dk>, "Martin K . Petersen" <martin.petersen@oracle.com>, Mike Snitzer <snitzer@redhat.com>, Eric Biggers <ebiggers@google.com>, linux-block@vger.kernel.org, dm-devel@redhat.com, Dmitry Monakhov <dmonakhov@openvz.org> Subject: Re: [dm-devel] [PATCH 5.20 1/4] block: add bio_rewind() API Date: Wed, 29 Jun 2022 09:02:58 +0800 [thread overview] Message-ID: <YrukwsgbFxd69wA1@T590> (raw) In-Reply-To: <20220628163617.h3bmq3opd7yuiaop@moria.home.lan> On Tue, Jun 28, 2022 at 12:36:17PM -0400, Kent Overstreet wrote: > On Tue, Jun 28, 2022 at 03:49:28PM +0800, Ming Lei wrote: > > On Tue, Jun 28, 2022 at 12:26:10AM -0400, Kent Overstreet wrote: > > > On Mon, Jun 27, 2022 at 03:36:22PM +0800, Ming Lei wrote: > > > > Not mention bio_iter, bvec_iter has been 32 bytes, which is too big to > > > > hold in per-io data structure. With this patch, 8bytes is enough > > > > to rewind one bio if the end sector is fixed. > > > > > > And with rewind, you're making an assumption about the state the iterator is > > > going to be in when the IO has completed. > > > > > > What if the iterator was never advanced? > > > > bio_rewind() works as expected if the iterator doesn't advance, since bytes > > between the recorded position and the end position isn't changed, same > > with the end position. > > > > > > > > So say you check for that by saving some other part of the iterator - but that > > > may have legitimately changed too, if the bio was redirected (bi_sector changes) > > > or trimmed (bi_size changes) > > > > > > I still think this is an inherently buggy interface, the way it's being proposed > > > to be used. > > > > The patch did mention that the interface should be for situation in which end > > sector of bio won't change. > > But that's an assumption that you simply can't make! Of course, we can, at least for this DM's use case, the bio is issued from FS or split from DM, and it won't be issued to underlying queue any more, and simply owned by DM core code. > > We allow block device drivers to be stacked in _any_ combination. After a bio is > completed it may have been partially advanced, fully advanced, trimmed, not > trimmed, anything - and bi_sector and thus also bio_end_sector() may have > changed, and will have if there's partition tables involved. How can bio (partial)advance change bio's end sector? bio end sector can be changed only when bio->bi_iter.bi_size is changed manually(include bio_trim), or ->bi_bdev is changed. But inside one driver, if the bio is owned by this driver(such as the driver is the finally layer request based driver), the assumption often can make. Thanks, Ming -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
next prev parent reply other threads:[~2022-06-29 1:03 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-24 14:12 [PATCH 5.20 0/4] block/dm: add bio_rewind for improving dm requeue Ming Lei 2022-06-24 14:12 ` [dm-devel] " Ming Lei 2022-06-24 14:12 ` [PATCH 5.20 1/4] block: add bio_rewind() API Ming Lei 2022-06-24 14:12 ` [dm-devel] " Ming Lei 2022-06-26 20:14 ` Kent Overstreet 2022-06-26 20:14 ` [dm-devel] " Kent Overstreet 2022-06-27 7:36 ` Ming Lei 2022-06-27 7:36 ` [dm-devel] " Ming Lei 2022-06-28 4:20 ` Kent Overstreet 2022-06-28 4:20 ` [dm-devel] " Kent Overstreet 2022-06-28 7:42 ` Ming Lei 2022-06-28 7:42 ` [dm-devel] " Ming Lei 2022-06-28 16:16 ` Kent Overstreet 2022-06-28 16:16 ` [dm-devel] " Kent Overstreet 2022-06-28 18:13 ` Jens Axboe 2022-06-28 18:13 ` [dm-devel] " Jens Axboe 2022-06-28 18:32 ` Kent Overstreet 2022-06-28 18:32 ` [dm-devel] " Kent Overstreet 2022-06-29 17:16 ` Jens Axboe 2022-06-29 17:16 ` [dm-devel] " Jens Axboe 2022-06-29 18:40 ` Kent Overstreet 2022-06-29 18:40 ` [dm-devel] " Kent Overstreet 2022-06-29 18:51 ` Bart Van Assche 2022-06-29 18:51 ` [dm-devel] " Bart Van Assche 2022-06-29 19:05 ` Kent Overstreet 2022-06-29 19:05 ` [dm-devel] " Kent Overstreet 2022-06-29 19:37 ` Bart Van Assche 2022-06-29 19:37 ` [dm-devel] " Bart Van Assche 2022-06-29 19:50 ` Kent Overstreet 2022-06-29 19:50 ` [dm-devel] " Kent Overstreet 2022-06-29 19:59 ` Kent Overstreet 2022-06-29 19:59 ` [dm-devel] " Kent Overstreet 2022-06-29 19:00 ` Jens Axboe 2022-06-29 19:00 ` [dm-devel] " Jens Axboe 2022-06-29 19:26 ` Kent Overstreet 2022-06-29 19:26 ` [dm-devel] " Kent Overstreet 2022-06-29 20:51 ` Jens Axboe 2022-06-29 20:51 ` [dm-devel] " Jens Axboe 2022-06-29 0:49 ` Ming Lei 2022-06-29 0:49 ` [dm-devel] " Ming Lei 2022-06-28 4:26 ` Kent Overstreet 2022-06-28 4:26 ` [dm-devel] " Kent Overstreet 2022-06-28 7:49 ` Ming Lei 2022-06-28 7:49 ` [dm-devel] " Ming Lei 2022-06-28 16:36 ` Kent Overstreet 2022-06-28 16:36 ` [dm-devel] " Kent Overstreet 2022-06-28 17:41 ` Mike Snitzer 2022-06-28 17:41 ` [dm-devel] " Mike Snitzer 2022-06-28 17:52 ` Kent Overstreet 2022-06-28 17:52 ` [dm-devel] " Kent Overstreet 2022-06-29 6:07 ` Mike Snitzer 2022-06-29 6:07 ` [dm-devel] " Mike Snitzer 2022-06-29 18:11 ` Kent Overstreet 2022-06-29 18:11 ` [dm-devel] " Kent Overstreet 2022-06-30 0:47 ` Ming Lei 2022-06-30 0:47 ` [dm-devel] " Ming Lei 2022-06-30 0:58 ` Kent Overstreet 2022-06-30 0:58 ` [dm-devel] " Kent Overstreet 2022-06-30 1:14 ` Kent Overstreet 2022-06-30 1:14 ` [dm-devel] " Kent Overstreet 2022-07-01 3:58 ` Ming Lei 2022-07-01 3:58 ` Ming Lei 2022-07-01 21:09 ` Kent Overstreet 2022-07-01 21:09 ` [dm-devel] " Kent Overstreet 2022-06-29 1:02 ` Ming Lei [this message] 2022-06-29 1:02 ` Ming Lei 2022-06-26 21:37 ` Eric Biggers 2022-06-26 21:37 ` Eric Biggers 2022-06-27 7:37 ` Ming Lei 2022-06-27 7:37 ` Ming Lei 2022-06-24 14:12 ` [PATCH 5.20 2/4] dm: add new helper for handling dm_io requeue Ming Lei 2022-06-24 14:12 ` [dm-devel] " Ming Lei 2022-06-24 14:12 ` [PATCH 5.20 3/4] dm: improve handling for DM_REQUEUE and AGAIN Ming Lei 2022-06-24 14:12 ` [dm-devel] " Ming Lei 2022-06-24 14:12 ` [PATCH 5.20 4/4] dm: add two stage requeue Ming Lei 2022-06-24 14:12 ` [dm-devel] " Ming Lei
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YrukwsgbFxd69wA1@T590 \ --to=ming.lei@redhat.com \ --cc=axboe@kernel.dk \ --cc=dm-devel@redhat.com \ --cc=dmonakhov@openvz.org \ --cc=ebiggers@google.com \ --cc=kent.overstreet@gmail.com \ --cc=linux-block@vger.kernel.org \ --cc=martin.petersen@oracle.com \ --cc=snitzer@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.