io-uring.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kanchan Joshi <joshiiitr@gmail.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Kanchan Joshi <joshi.k@samsung.com>, Jens Axboe <axboe@kernel.dk>,
	Keith Busch <kbusch@kernel.org>,
	io-uring@vger.kernel.org, linux-nvme@lists.infradead.org,
	anuj20.g@samsung.com, Javier Gonzalez <javier.gonz@samsung.com>,
	hare@suse.de
Subject: Re: [RFC PATCH 2/6] nvme: wire-up support for async-passthru on char-device.
Date: Wed, 22 Sep 2021 12:49:52 +0530	[thread overview]
Message-ID: <CA+1E3rLeM=GZn1gR_KN7b6o8LHDistp1ZoHBb0N54ayK-2+tPA@mail.gmail.com> (raw)
In-Reply-To: <20210908061530.GA28505@lst.de>

I am sorry for taking longer than I should have.

On Wed, Sep 8, 2021 at 11:45 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Sep 07, 2021 at 09:50:27PM +0530, Kanchan Joshi wrote:
> > > A few other notes:
> > >
> > >  - I suspect the ioctl_cmd really should move into the core using_cmd
> > >    infrastructure
> >
> > Yes, that seems possible by creating that field outside by combining
> > "op" and "unused" below.
> > +struct io_uring_cmd {
> > + struct file *file;
> > + __u16 op;
> > + __u16 unused;
> > + __u32 len;
> > + __u64 pdu[5]; /* 40 bytes available inline for free use */
> > +};
>
> Two different issues here:
>
>  - the idea of having a two layer indirection with op and a cmd doesn't
>    really make much sense
>  - if we want to avoid conflicts using 32-bit probably makes sense
>
> So I'd turn op and unused into a single cmd field, use the ioctl encoding
> macros for it (but preferably pick different numbers than the existing
> ioctls).

I was thinking along the same lines, except the "picking different
numbers than existing ioctls" part.
Does that mean adding a new IOCTL for each operation which requires
async transport?

> > >  - that whole mix of user space interface and internal data in the
> > >    ->pdu field is a mess.  What is the problem with deferring the
> > >    request freeing into the user context, which would clean up
> > >    quite a bit of that, especially if io_uring_cmd grows a private
> > >    field.
> >
> > That mix isn't great but the attempt was to save the allocation.
> > And I was not very sure if it'd be fine to defer freeing the request
> > until task-work fires up.
>
> What would be the problem with the delaying?

When we free the request, the tag is also freed, and that may enable
someone else to pump more IO.
Pushing freeing of requests to user-context seemed like delaying that part.
If you think that is a misplaced concern, I can change.
The changed structure will look like this -

struct nvme_uring_cmd {
       __u32   ioctl_cmd;
       __u32   unused1;
       void __user *argp;
      union {
                struct bio *bio;
                struct request *req;
             };
       void *meta;
};
cmd->bio will be freed in nvme-completion while cmd->req will be freed
in user context.
Since we have the request intact, we will not store "u64 result; int
status;" anymore and overall there will be a reduction of 4 bytes in
size of nvme_uring_cmd.
Would you prefer this way?

> > Even if we take that route, we would still need a place to store bio
> > pointer (hopefully meta pointer can be extracted out of bio).
> > Do you see it differently?
>
> We don't need the bio pointer at all.  The old passthrough code needed
> it when we still used block layer bonuce buffering for it.  But that
> bounce buffering for passthrough commands got removed a while ago,
> and even before nvme never used it.

nvme_submit_user_cmd() calls blk_rq_map_user(), which sets up req->bio
(and that is regardless of bounce buffering I suppose).
For sync-ioctl, this bio pointer is locally stored and that is used to
free the bio post completion.
For async-ioctl too, we need some place to store this.
So I could not connect this to bounce buffering (alone). Am I missing
your point?

One of the way could be to change blk_update_request() to avoid
setting req->bio to NULL. But perhaps that may invite more troubles,
and we are not saving anything: bio-pointer is inside the union anyway
(in above struct nvme_uring_cmd).


-- 
Kanchan

  reply	other threads:[~2021-09-22  7:20 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20210805125910epcas5p1100e7093dd2b1ac5bbb751331e2ded23@epcas5p1.samsung.com>
2021-08-05 12:55 ` [RFC PATCH 0/6] Fixed-buffers io_uring passthrough over nvme-char Kanchan Joshi
     [not found]   ` <CGME20210805125917epcas5p4f75c9423a7b886dc79500901cc8f55ab@epcas5p4.samsung.com>
2021-08-05 12:55     ` [RFC PATCH 1/6] io_uring: add infra for uring_cmd completion in submitter-task Kanchan Joshi
     [not found]   ` <CGME20210805125923epcas5p10e6c1b95475440be68f58244d5a3cb9a@epcas5p1.samsung.com>
2021-08-05 12:55     ` [RFC PATCH 2/6] nvme: wire-up support for async-passthru on char-device Kanchan Joshi
2021-09-07  7:46       ` Christoph Hellwig
2021-09-07 16:20         ` Kanchan Joshi
2021-09-08  6:15           ` Christoph Hellwig
2021-09-22  7:19             ` Kanchan Joshi [this message]
     [not found]   ` <CGME20210805125927epcas5p28f3413fe3d0a2baed37a05453df0d482@epcas5p2.samsung.com>
2021-08-05 12:55     ` [RFC PATCH 3/6] io_uring: mark iopoll not supported for uring-cmd Kanchan Joshi
     [not found]   ` <CGME20210805125931epcas5p259fec172085ea34fdbf5a1c1f8da5e90@epcas5p2.samsung.com>
2021-08-05 12:55     ` [RFC PATCH 4/6] io_uring: add helper for fixed-buffer uring-cmd Kanchan Joshi
2021-09-07  7:47       ` Christoph Hellwig
     [not found]   ` <CGME20210805125934epcas5p4ff88e95d558ad9f65d77a888a4211b18@epcas5p4.samsung.com>
2021-08-05 12:55     ` [RFC PATCH 5/6] io_uring: add support for uring_cmd with fixed-buffer Kanchan Joshi
2021-09-07  7:48       ` Christoph Hellwig
2021-09-07 16:29         ` Kanchan Joshi
     [not found]   ` <CGME20210805125937epcas5p15667b460e28d87bd40400f69005aafe3@epcas5p1.samsung.com>
2021-08-05 12:55     ` [RFC PATCH 6/6] nvme: enable passthrough " Kanchan Joshi
2021-09-07  7:50       ` Christoph Hellwig
2021-09-07 16:47         ` Kanchan Joshi
2021-09-08  6:16           ` Christoph Hellwig
2021-09-07  7:10   ` [RFC PATCH 0/6] Fixed-buffers io_uring passthrough over nvme-char Christoph Hellwig
2021-09-07 12:38     ` Jens Axboe

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='CA+1E3rLeM=GZn1gR_KN7b6o8LHDistp1ZoHBb0N54ayK-2+tPA@mail.gmail.com' \
    --to=joshiiitr@gmail.com \
    --cc=anuj20.g@samsung.com \
    --cc=axboe@kernel.dk \
    --cc=hare@suse.de \
    --cc=hch@lst.de \
    --cc=io-uring@vger.kernel.org \
    --cc=javier.gonz@samsung.com \
    --cc=joshi.k@samsung.com \
    --cc=kbusch@kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --subject='Re: [RFC PATCH 2/6] nvme: wire-up support for async-passthru on char-device.' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).