io-uring.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stefan Metzmacher <metze@samba.org>
To: Jens Axboe <axboe@kernel.dk>, "Darrick J. Wong" <djwong@kernel.org>
Cc: io-uring@vger.kernel.org
Subject: Re: [PATCH 2/5] io_uring: add support for IORING_OP_URING_CMD
Date: Mon, 22 Feb 2021 21:04:07 +0100	[thread overview]
Message-ID: <bcce9dd6-8222-6dc5-ad4f-5a68ac3ca902@samba.org> (raw)
In-Reply-To: <58fe1e10-ed71-53f9-c47c-5e64066d3290@kernel.dk>


[-- Attachment #1.1: Type: text/plain, Size: 3670 bytes --]

Hi Jens,

>> I've been thinking along the same lines, because having a sparse sqe layout
>> for the uring cmd is a pain. I do think 'personality' is a bit too specific
>> to be part of the shared space, that should probably belong in the pdu
>> instead if the user needs it. One thing they all have in common is that they'd
>> need a sub-command, so why not make that u16 that?
>>
>> There's also the option of simply saying that the uring_cmd sqe is just
>> a different type, ala:
>>
>> struct io_uring_cmd_sqe {
>> 	__u8	opcode;		/* IO_OP_URING_CMD */
>> 	__u8	flags;
>> 	__u16	target_op;
>> 	__s32	fd;
>> 	__u64	user_data;
>> 	strut io_uring_cmd_pdu cmd_pdu;
>> };
>>
>> which is essentially the same as your suggestion in terms of layout
>> (because that is the one that makes the most sense), we just don't try
>> and shoe-horn it into the existing sqe. As long as we overlap
>> opcode/flags, then init is fine. And past init, sqe is already consumed.
>>
>> Haven't tried and wire that up yet, and it may just be that the simple
>> layout change you did is just easier to deal with. The important part
>> here is the layout, and I certainly think we should do that. There's
>> effectively 54 bytes of data there, if you include the target op and fd
>> as part of that space. 48 fully usable for whatever.
> 
> OK, folded in some of your stuff, and pushed out a new branch. Find it
> here:
> 
> https://git.kernel.dk/cgit/linux-block/log/?h=io_uring-fops.v3
> 
> I did notice while doing so that you put the issue flags in the cmd,
> I've made them external again. Just seems cleaner to me, otherwise
> you'd have to modify the command for reissue rather than just
> pass in the flags directly.

I think the first two commits need more verbose comments, which clearly
document the uring_cmd() API.

Event before uring_cmd(), it's really not clear to me why we have
'enum io_uring_cmd_flags', as 'enum'.
As it seems to be use it as 'flags' (IO_URING_F_NONBLOCK|IO_URING_F_COMPLETE_DEFER).

With uring_cmd() it's not clear what the backend is supposed to do with these flags.

I'd assume that uring_cmd() would per definition never block and go async itself,
by returning -EIOCBQUEUED. And a single &req->uring_cmd is only ever passed once to
uring_cmd() without any retry.

It's also not clear if IOSQE_ASYNC should have any impact.

I think we also need a way to pass IORING_OP_ASYNC_CANCEL down.

> I also retained struct file * in the cmd - that's a requirement for
> the layout of io_kiocb, so might as well keep it in there and not
> pass in the file. Plus that one won't ever change...

Ah, ok.

> Since we just need that one branch in req init, I do think that your
> suggestion of just modifying io_uring_sqe is the way to go. So that's
> what the above branch does.

Thanks! I think it's much easier to handle the personality logic in
the core only.

For fixed files or fixed buffers I think helper functions like this:

struct file *io_uring_cmd_get_file(struct io_uring_cmd *cmd, int fd, bool fixed);

And similar functions for io_buffer_select or io_import_fixed.

> I tested the block side, and it works for getting the bs of the
> device. That's all the testing that has been done so far :-)

I've added EXPORT_SYMBOL(io_uring_cmd_done); and split your net patch,
similar to the two block patches. So we can better isolate the core
from the first consumers.

See https://git.samba.org/?p=metze/linux/wip.git;a=shortlog;h=refs/heads/io_uring-fops.v3

> Comments welcome! Would like to move this one forward and hopefully
> target 5.13 for it.

Great!

metze



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2021-02-22 20:04 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-27 21:25 [PATCHSET RFC 0/5] file_operations based io_uring commands Jens Axboe
2021-01-27 21:25 ` [PATCH 1/5] fs: add file_operations->uring_cmd() Jens Axboe
2021-01-27 21:25 ` [PATCH 2/5] io_uring: add support for IORING_OP_URING_CMD Jens Axboe
2021-01-28  0:38   ` Darrick J. Wong
2021-01-28  1:45     ` Jens Axboe
2021-01-28  2:19       ` Jens Axboe
2021-02-20  3:57         ` Stefan Metzmacher
2021-02-20 14:50           ` Jens Axboe
2021-02-20 16:45             ` Jens Axboe
2021-02-22 20:04               ` Stefan Metzmacher [this message]
2021-02-22 20:14                 ` Jens Axboe
2021-02-23  8:14                   ` Stefan Metzmacher
2021-02-23 13:21                     ` Pavel Begunkov
2021-01-27 21:25 ` [PATCH 3/5] block: wire up support for file_operations->uring_cmd() Jens Axboe
2021-01-27 21:25 ` [PATCH 4/5] block: add example ioctl Jens Axboe
2021-01-27 21:25 ` [PATCH 5/5] net: wire up support for file_operations->uring_cmd() 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=bcce9dd6-8222-6dc5-ad4f-5a68ac3ca902@samba.org \
    --to=metze@samba.org \
    --cc=axboe@kernel.dk \
    --cc=djwong@kernel.org \
    --cc=io-uring@vger.kernel.org \
    --subject='Re: [PATCH 2/5] io_uring: add support for IORING_OP_URING_CMD' \
    /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).