All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ming Lei <ming.lei@redhat.com>
To: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org, dm-devel@redhat.com,
	Christoph Hellwig <hch@lst.de>, Mike Snitzer <snitzer@redhat.com>
Subject: Re: [dm-devel] [PATCH v5 2/2] dm: support bio polling
Date: Mon, 7 Mar 2022 11:39:35 +0800	[thread overview]
Message-ID: <YiV+d+B2+o7q63Bm@T590> (raw)
In-Reply-To: <YiVw2y1eTcXrsdME@T590>

On Mon, Mar 07, 2022 at 10:41:31AM +0800, Ming Lei wrote:
> On Sun, Mar 06, 2022 at 07:25:11PM -0700, Jens Axboe wrote:
> > On 3/6/22 7:20 PM, Ming Lei wrote:
> > > On Sun, Mar 06, 2022 at 06:48:15PM -0700, Jens Axboe wrote:
> > >> On 3/6/22 2:29 AM, Christoph Hellwig wrote:
> > >>>> +/*
> > >>>> + * Reuse ->bi_end_io as hlist head for storing all dm_io instances
> > >>>> + * associated with this bio, and this bio's bi_end_io has to be
> > >>>> + * stored in one of 'dm_io' instance first.
> > >>>> + */
> > >>>> +static inline struct hlist_head *dm_get_bio_hlist_head(struct bio *bio)
> > >>>> +{
> > >>>> +	WARN_ON_ONCE(!(bio->bi_opf & REQ_DM_POLL_LIST));
> > >>>> +
> > >>>> +	return (struct hlist_head *)&bio->bi_end_io;
> > >>>> +}
> > >>>
> > >>> So this reuse is what I really hated.  I still think we should be able
> > >>> to find space in the bio by creatively shifting fields around to just
> > >>> add the hlist there directly, which would remove the need for this
> > >>> override and more importantly the quite cumbersome saving and restoring
> > >>> of the end_io handler.
> > >>
> > >> If it's possible, then that would be preferable. But I don't think
> > >> that's going to be easy to do...
> > > 
> > > I agree, now basically there isn't gap inside bio, so either adding one
> > > new field or reusing one existed field...
> > 
> > There'd no amount of re-arranging that'll free up 8 bytes, that's just
> > not happening. I'm not a huge fan of growing struct bio for that, and
> > the oddity here is mostly (to me) that ->bi_end_io is the one overlayed.
> > That would usually belong to the owner of the bio.
> > 
> > Maybe some commenting would help?
> 
> OK, ->bi_end_io is safe because it is only called until the bio is
> ended, so we can retrieve the list head and recover ->bi_end_io before
> polling.

->bi_private can be reused too, is that better?

Yeah, both belong to owner(higher level storage), then block layer can't touch
them inside submit_bio_noacct(), that is also why this trick is safe.

Thanks,
Ming
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


WARNING: multiple messages have this Message-ID (diff)
From: Ming Lei <ming.lei@redhat.com>
To: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>, Mike Snitzer <snitzer@redhat.com>,
	dm-devel@redhat.com, linux-block@vger.kernel.org
Subject: Re: [PATCH v5 2/2] dm: support bio polling
Date: Mon, 7 Mar 2022 11:39:35 +0800	[thread overview]
Message-ID: <YiV+d+B2+o7q63Bm@T590> (raw)
In-Reply-To: <YiVw2y1eTcXrsdME@T590>

On Mon, Mar 07, 2022 at 10:41:31AM +0800, Ming Lei wrote:
> On Sun, Mar 06, 2022 at 07:25:11PM -0700, Jens Axboe wrote:
> > On 3/6/22 7:20 PM, Ming Lei wrote:
> > > On Sun, Mar 06, 2022 at 06:48:15PM -0700, Jens Axboe wrote:
> > >> On 3/6/22 2:29 AM, Christoph Hellwig wrote:
> > >>>> +/*
> > >>>> + * Reuse ->bi_end_io as hlist head for storing all dm_io instances
> > >>>> + * associated with this bio, and this bio's bi_end_io has to be
> > >>>> + * stored in one of 'dm_io' instance first.
> > >>>> + */
> > >>>> +static inline struct hlist_head *dm_get_bio_hlist_head(struct bio *bio)
> > >>>> +{
> > >>>> +	WARN_ON_ONCE(!(bio->bi_opf & REQ_DM_POLL_LIST));
> > >>>> +
> > >>>> +	return (struct hlist_head *)&bio->bi_end_io;
> > >>>> +}
> > >>>
> > >>> So this reuse is what I really hated.  I still think we should be able
> > >>> to find space in the bio by creatively shifting fields around to just
> > >>> add the hlist there directly, which would remove the need for this
> > >>> override and more importantly the quite cumbersome saving and restoring
> > >>> of the end_io handler.
> > >>
> > >> If it's possible, then that would be preferable. But I don't think
> > >> that's going to be easy to do...
> > > 
> > > I agree, now basically there isn't gap inside bio, so either adding one
> > > new field or reusing one existed field...
> > 
> > There'd no amount of re-arranging that'll free up 8 bytes, that's just
> > not happening. I'm not a huge fan of growing struct bio for that, and
> > the oddity here is mostly (to me) that ->bi_end_io is the one overlayed.
> > That would usually belong to the owner of the bio.
> > 
> > Maybe some commenting would help?
> 
> OK, ->bi_end_io is safe because it is only called until the bio is
> ended, so we can retrieve the list head and recover ->bi_end_io before
> polling.

->bi_private can be reused too, is that better?

Yeah, both belong to owner(higher level storage), then block layer can't touch
them inside submit_bio_noacct(), that is also why this trick is safe.

Thanks,
Ming


  reply	other threads:[~2022-03-07  3:40 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-05  2:08 [dm-devel] [PATCH v5 0/2] block/dm: support bio polling Mike Snitzer
2022-03-05  2:08 ` Mike Snitzer
2022-03-05  2:08 ` [dm-devel] [PATCH v5 1/2] block: add ->poll_bio to block_device_operations Mike Snitzer
2022-03-05  2:08   ` Mike Snitzer
2022-03-06  9:27   ` [dm-devel] " Christoph Hellwig
2022-03-06  9:27     ` Christoph Hellwig
2022-03-05  2:08 ` [dm-devel] [PATCH v5 2/2] dm: support bio polling Mike Snitzer
2022-03-05  2:08   ` Mike Snitzer
2022-03-06  9:29   ` [dm-devel] " Christoph Hellwig
2022-03-06  9:29     ` Christoph Hellwig
2022-03-07  1:48     ` [dm-devel] " Jens Axboe
2022-03-07  1:48       ` Jens Axboe
2022-03-07  2:20       ` [dm-devel] " Ming Lei
2022-03-07  2:20         ` Ming Lei
2022-03-07  2:25         ` [dm-devel] " Jens Axboe
2022-03-07  2:25           ` Jens Axboe
2022-03-07  2:41           ` [dm-devel] " Ming Lei
2022-03-07  2:41             ` Ming Lei
2022-03-07  3:39             ` Ming Lei [this message]
2022-03-07  3:39               ` 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=YiV+d+B2+o7q63Bm@T590 \
    --to=ming.lei@redhat.com \
    --cc=axboe@kernel.dk \
    --cc=dm-devel@redhat.com \
    --cc=hch@lst.de \
    --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: link
Be 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.