From: Ming Lei <ming.lei@redhat.com> To: Christoph Hellwig <hch@lst.de> Cc: Jens Axboe <axboe@kernel.dk>, Mike Snitzer <snitzer@redhat.com>, linux-block@vger.kernel.org, Jeffle Xu <jefflexu@linux.alibaba.com>, dm-devel@redhat.com, Hannes Reinecke <hare@suse.de> Subject: Re: [RFC PATCH V2 3/3] dm: support bio polling Date: Mon, 21 Jun 2021 17:09:49 +0800 [thread overview] Message-ID: <YNBXXVD9lko84IEZ@T590> (raw) In-Reply-To: <20210621073656.GB6896@lst.de> On Mon, Jun 21, 2021 at 09:36:56AM +0200, Christoph Hellwig wrote: > On Thu, Jun 17, 2021 at 06:35:49PM +0800, Ming Lei wrote: > > + /* > > + * Only support bio polling for normal IO, and the target io is > > + * exactly inside the dm io instance > > + */ > > + ci->io->submit_as_polled = !!(ci->bio->bi_opf & REQ_POLLED); > > Nit: the !! is not needed. OK. > > > @@ -1608,6 +1625,22 @@ static void init_clone_info(struct clone_info *ci, struct mapped_device *md, > > ci->map = map; > > ci->io = alloc_io(md, bio); > > ci->sector = bio->bi_iter.bi_sector; > > + > > + if (bio->bi_opf & REQ_POLLED) { > > + INIT_HLIST_NODE(&ci->io->node); > > + > > + /* > > + * Save .bi_end_io into dm_io, so that we can reuse .bi_end_io > > + * for storing dm_io list > > + */ > > + if (bio->bi_opf & REQ_SAVED_END_IO) { > > + ci->io->saved_bio_end_io = NULL; > > So if it already was saved the list gets cleared here? Can you explain > this logic a little more? Inside dm_poll_bio() we recognize non-NULL ->saved_bio_end_io as valid, so it has to be initialized it here. > > > + } else { > > + ci->io->saved_bio_end_io = bio->bi_end_io; > > + INIT_HLIST_HEAD((struct hlist_head *)&bio->bi_end_io); > > I think you want to hide these casts in helpers that clearly document > why this is safe rather than sprinkling the casts all over the code. > I also wonder if there is any better way to structur this. OK, I will add a helper of dm_get_bio_hlist_head() with comment. > > > +static int dm_poll_bio(struct bio *bio, unsigned int flags) > > +{ > > + struct dm_io *io; > > + void *saved_bi_end_io = NULL; > > + struct hlist_head tmp = HLIST_HEAD_INIT; > > + struct hlist_head *head = (struct hlist_head *)&bio->bi_end_io; > > + struct hlist_node *next; > > + > > + /* > > + * This bio can be submitted from FS as POLLED so that FS may keep > > + * polling even though the flag is cleared by bio splitting or > > + * requeue, so return immediately. > > + */ > > + if (!(bio->bi_opf & REQ_POLLED)) > > + return 0; > > I can't really parse the comment, can you explain this a little more? > But if we need this check, shouldn't it move to bio_poll()? Upper layer keeps to poll one bio with POLLED, but the flag can be cleared by driver or block layer. Once it is cleared, we should return immediately. Yeah, we can move it to bio_poll(). > > > + hlist_for_each_entry(io, &tmp, node) { > > + if (io->saved_bio_end_io && !saved_bi_end_io) { > > + saved_bi_end_io = io->saved_bio_end_io; > > + break; > > + } > > + } > > So it seems like you don't use bi_cookie at all. Why not turn > bi_cookie into a union to stash the hlist_head and use that? hlist_head is 'void *', but ->bi_cookie is 'unsigned int'. Thanks, Ming
WARNING: multiple messages have this Message-ID (diff)
From: Ming Lei <ming.lei@redhat.com> To: Christoph Hellwig <hch@lst.de> Cc: Jens Axboe <axboe@kernel.dk>, Mike Snitzer <snitzer@redhat.com>, linux-block@vger.kernel.org, dm-devel@redhat.com, Jeffle Xu <jefflexu@linux.alibaba.com> Subject: Re: [dm-devel] [RFC PATCH V2 3/3] dm: support bio polling Date: Mon, 21 Jun 2021 17:09:49 +0800 [thread overview] Message-ID: <YNBXXVD9lko84IEZ@T590> (raw) In-Reply-To: <20210621073656.GB6896@lst.de> On Mon, Jun 21, 2021 at 09:36:56AM +0200, Christoph Hellwig wrote: > On Thu, Jun 17, 2021 at 06:35:49PM +0800, Ming Lei wrote: > > + /* > > + * Only support bio polling for normal IO, and the target io is > > + * exactly inside the dm io instance > > + */ > > + ci->io->submit_as_polled = !!(ci->bio->bi_opf & REQ_POLLED); > > Nit: the !! is not needed. OK. > > > @@ -1608,6 +1625,22 @@ static void init_clone_info(struct clone_info *ci, struct mapped_device *md, > > ci->map = map; > > ci->io = alloc_io(md, bio); > > ci->sector = bio->bi_iter.bi_sector; > > + > > + if (bio->bi_opf & REQ_POLLED) { > > + INIT_HLIST_NODE(&ci->io->node); > > + > > + /* > > + * Save .bi_end_io into dm_io, so that we can reuse .bi_end_io > > + * for storing dm_io list > > + */ > > + if (bio->bi_opf & REQ_SAVED_END_IO) { > > + ci->io->saved_bio_end_io = NULL; > > So if it already was saved the list gets cleared here? Can you explain > this logic a little more? Inside dm_poll_bio() we recognize non-NULL ->saved_bio_end_io as valid, so it has to be initialized it here. > > > + } else { > > + ci->io->saved_bio_end_io = bio->bi_end_io; > > + INIT_HLIST_HEAD((struct hlist_head *)&bio->bi_end_io); > > I think you want to hide these casts in helpers that clearly document > why this is safe rather than sprinkling the casts all over the code. > I also wonder if there is any better way to structur this. OK, I will add a helper of dm_get_bio_hlist_head() with comment. > > > +static int dm_poll_bio(struct bio *bio, unsigned int flags) > > +{ > > + struct dm_io *io; > > + void *saved_bi_end_io = NULL; > > + struct hlist_head tmp = HLIST_HEAD_INIT; > > + struct hlist_head *head = (struct hlist_head *)&bio->bi_end_io; > > + struct hlist_node *next; > > + > > + /* > > + * This bio can be submitted from FS as POLLED so that FS may keep > > + * polling even though the flag is cleared by bio splitting or > > + * requeue, so return immediately. > > + */ > > + if (!(bio->bi_opf & REQ_POLLED)) > > + return 0; > > I can't really parse the comment, can you explain this a little more? > But if we need this check, shouldn't it move to bio_poll()? Upper layer keeps to poll one bio with POLLED, but the flag can be cleared by driver or block layer. Once it is cleared, we should return immediately. Yeah, we can move it to bio_poll(). > > > + hlist_for_each_entry(io, &tmp, node) { > > + if (io->saved_bio_end_io && !saved_bi_end_io) { > > + saved_bi_end_io = io->saved_bio_end_io; > > + break; > > + } > > + } > > So it seems like you don't use bi_cookie at all. Why not turn > bi_cookie into a union to stash the hlist_head and use that? hlist_head is 'void *', but ->bi_cookie is 'unsigned int'. Thanks, Ming -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
next prev parent reply other threads:[~2021-06-21 9:10 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-17 10:35 [RFC PATCH V2 0/3] block/dm: support bio polling Ming Lei 2021-06-17 10:35 ` [dm-devel] " Ming Lei 2021-06-17 10:35 ` [RFC PATCH V2 1/3] block: add helper of blk_queue_poll Ming Lei 2021-06-17 10:35 ` [dm-devel] " Ming Lei 2021-06-21 7:20 ` Christoph Hellwig 2021-06-21 7:20 ` [dm-devel] " Christoph Hellwig 2021-06-21 8:38 ` Ming Lei 2021-06-21 8:38 ` [dm-devel] " Ming Lei 2021-06-17 10:35 ` [RFC PATCH V2 2/3] block: add ->poll_bio to block_device_operations Ming Lei 2021-06-17 10:35 ` [dm-devel] " Ming Lei 2021-06-21 7:25 ` Christoph Hellwig 2021-06-21 7:25 ` [dm-devel] " Christoph Hellwig 2021-06-21 8:41 ` Ming Lei 2021-06-21 8:41 ` [dm-devel] " Ming Lei 2021-06-17 10:35 ` [RFC PATCH V2 3/3] dm: support bio polling Ming Lei 2021-06-17 10:35 ` [dm-devel] " Ming Lei 2021-06-17 23:08 ` Ming Lei 2021-06-17 23:08 ` [dm-devel] " Ming Lei 2021-06-18 8:19 ` JeffleXu 2021-06-18 8:19 ` JeffleXu 2021-06-18 13:29 ` Ming Lei 2021-06-18 13:29 ` Ming Lei 2021-06-18 14:39 ` Ming Lei 2021-06-18 14:39 ` Ming Lei 2021-06-18 20:56 ` Mike Snitzer 2021-06-18 20:56 ` [dm-devel] " Mike Snitzer 2021-06-19 0:27 ` Ming Lei 2021-06-19 0:27 ` [dm-devel] " Ming Lei 2021-06-21 1:32 ` JeffleXu 2021-06-21 1:32 ` [dm-devel] " JeffleXu 2021-06-21 11:33 ` JeffleXu 2021-06-21 11:33 ` JeffleXu 2021-06-21 14:04 ` Ming Lei 2021-06-21 14:04 ` Ming Lei 2021-06-22 2:26 ` JeffleXu 2021-06-22 2:26 ` JeffleXu 2021-06-22 2:45 ` Ming Lei 2021-06-22 2:45 ` Ming Lei 2021-06-22 7:45 ` JeffleXu 2021-06-22 7:45 ` JeffleXu 2021-06-30 8:30 ` Ming Lei 2021-06-30 8:30 ` Ming Lei 2021-06-21 7:36 ` Christoph Hellwig 2021-06-21 7:36 ` [dm-devel] " Christoph Hellwig 2021-06-21 9:09 ` Ming Lei [this message] 2021-06-21 9:09 ` 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=YNBXXVD9lko84IEZ@T590 \ --to=ming.lei@redhat.com \ --cc=axboe@kernel.dk \ --cc=dm-devel@redhat.com \ --cc=hare@suse.de \ --cc=hch@lst.de \ --cc=jefflexu@linux.alibaba.com \ --cc=linux-block@vger.kernel.org \ --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.