| Message ID | 20250317135742.4331-5-sidong.yang@furiosa.ai (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | introduce io_uring_cmd_import_fixed_vec | expand |
On Mon, Mar 17, 2025 at 7:00 AM Sidong Yang <sidong.yang@furiosa.ai> wrote: > > This patch introduces btrfs_uring_import_iovec(). In encoded read/write > with uring cmd, it uses import_iovec without supporting fixed buffer. > btrfs_using_import_iovec() could use fixed buffer if cmd flags has > IORING_URING_CMD_FIXED. > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> > --- > fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 6c18bad53cd3..a7b52fd99059 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { > struct iov_iter iter; > }; > > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, > + unsigned int issue_flags, int rw) > +{ > + struct btrfs_uring_encoded_data *data = > + io_uring_cmd_get_async_data(cmd)->op_data; > + int ret; > + > + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { > + data->iov = NULL; > + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, > + data->args.iovcnt, > + ITER_DEST, issue_flags, Why ITER_DEST instead of rw? Best, Caleb > + &data->iter); > + } else { > + data->iov = data->iovstack; > + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, > + ARRAY_SIZE(data->iovstack), &data->iov, > + &data->iter); > + } > + return ret; > +} > + > static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags) > { > size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args, flags); > @@ -4874,10 +4896,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue > goto out_acct; > } > > - data->iov = data->iovstack; > - ret = import_iovec(ITER_DEST, data->args.iov, data->args.iovcnt, > - ARRAY_SIZE(data->iovstack), &data->iov, > - &data->iter); > + ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_DEST); > if (ret < 0) > goto out_acct; > > @@ -5022,10 +5041,7 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu > if (data->args.len > data->args.unencoded_len - data->args.unencoded_offset) > goto out_acct; > > - data->iov = data->iovstack; > - ret = import_iovec(ITER_SOURCE, data->args.iov, data->args.iovcnt, > - ARRAY_SIZE(data->iovstack), &data->iov, > - &data->iter); > + ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_SOURCE); > if (ret < 0) > goto out_acct; > > -- > 2.43.0 > >
On 3/17/25 7:57 AM, Sidong Yang wrote: > This patch introduces btrfs_uring_import_iovec(). In encoded read/write > with uring cmd, it uses import_iovec without supporting fixed buffer. > btrfs_using_import_iovec() could use fixed buffer if cmd flags has > IORING_URING_CMD_FIXED. > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> > --- > fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 6c18bad53cd3..a7b52fd99059 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { > struct iov_iter iter; > }; > > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, > + unsigned int issue_flags, int rw) > +{ > + struct btrfs_uring_encoded_data *data = > + io_uring_cmd_get_async_data(cmd)->op_data; > + int ret; > + > + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { > + data->iov = NULL; > + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, > + data->args.iovcnt, > + ITER_DEST, issue_flags, > + &data->iter); > + } else { > + data->iov = data->iovstack; > + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, > + ARRAY_SIZE(data->iovstack), &data->iov, > + &data->iter); > + } > + return ret; > +} How can 'cmd' be NULL here?
On Mon, Mar 17, 2025 at 08:37:04AM -0700, Caleb Sander Mateos wrote: > On Mon, Mar 17, 2025 at 7:00 AM Sidong Yang <sidong.yang@furiosa.ai> wrote: > > > > This patch introduces btrfs_uring_import_iovec(). In encoded read/write > > with uring cmd, it uses import_iovec without supporting fixed buffer. > > btrfs_using_import_iovec() could use fixed buffer if cmd flags has > > IORING_URING_CMD_FIXED. > > > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> > > --- > > fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- > > 1 file changed, 24 insertions(+), 8 deletions(-) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index 6c18bad53cd3..a7b52fd99059 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { > > struct iov_iter iter; > > }; > > > > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, > > + unsigned int issue_flags, int rw) > > +{ > > + struct btrfs_uring_encoded_data *data = > > + io_uring_cmd_get_async_data(cmd)->op_data; > > + int ret; > > + > > + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { > > + data->iov = NULL; > > + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, > > + data->args.iovcnt, > > + ITER_DEST, issue_flags, > > Why ITER_DEST instead of rw? Oh, it's a mistake. It should be rw. Thanks, Sidong > > Best, > Caleb > > > + &data->iter); > > + } else { > > + data->iov = data->iovstack; > > + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, > > + ARRAY_SIZE(data->iovstack), &data->iov, > > + &data->iter); > > + } > > + return ret; > > +} > > + > > static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags) > > { > > size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args, flags); > > @@ -4874,10 +4896,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue > > goto out_acct; > > } > > > > - data->iov = data->iovstack; > > - ret = import_iovec(ITER_DEST, data->args.iov, data->args.iovcnt, > > - ARRAY_SIZE(data->iovstack), &data->iov, > > - &data->iter); > > + ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_DEST); > > if (ret < 0) > > goto out_acct; > > > > @@ -5022,10 +5041,7 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu > > if (data->args.len > data->args.unencoded_len - data->args.unencoded_offset) > > goto out_acct; > > > > - data->iov = data->iovstack; > > - ret = import_iovec(ITER_SOURCE, data->args.iov, data->args.iovcnt, > > - ARRAY_SIZE(data->iovstack), &data->iov, > > - &data->iter); > > + ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_SOURCE); > > if (ret < 0) > > goto out_acct; > > > > -- > > 2.43.0 > > > >
On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote: > On 3/17/25 7:57 AM, Sidong Yang wrote: > > This patch introduces btrfs_uring_import_iovec(). In encoded read/write > > with uring cmd, it uses import_iovec without supporting fixed buffer. > > btrfs_using_import_iovec() could use fixed buffer if cmd flags has > > IORING_URING_CMD_FIXED. > > > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> > > --- > > fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- > > 1 file changed, 24 insertions(+), 8 deletions(-) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index 6c18bad53cd3..a7b52fd99059 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { > > struct iov_iter iter; > > }; > > > > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, > > + unsigned int issue_flags, int rw) > > +{ > > + struct btrfs_uring_encoded_data *data = > > + io_uring_cmd_get_async_data(cmd)->op_data; > > + int ret; > > + > > + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { > > + data->iov = NULL; > > + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, > > + data->args.iovcnt, > > + ITER_DEST, issue_flags, > > + &data->iter); > > + } else { > > + data->iov = data->iovstack; > > + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, > > + ARRAY_SIZE(data->iovstack), &data->iov, > > + &data->iter); > > + } > > + return ret; > > +} > > How can 'cmd' be NULL here? It seems that there is no checkes for 'cmd' before and it works same as before. But I think it's better to add a check in function start for safety. > > > -- > Jens Axboe >
On 3/18/25 01:58, Sidong Yang wrote: > On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote: >> On 3/17/25 7:57 AM, Sidong Yang wrote: >>> This patch introduces btrfs_uring_import_iovec(). In encoded read/write >>> with uring cmd, it uses import_iovec without supporting fixed buffer. >>> btrfs_using_import_iovec() could use fixed buffer if cmd flags has >>> IORING_URING_CMD_FIXED. >>> >>> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> >>> --- >>> fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- >>> 1 file changed, 24 insertions(+), 8 deletions(-) >>> >>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >>> index 6c18bad53cd3..a7b52fd99059 100644 >>> --- a/fs/btrfs/ioctl.c >>> +++ b/fs/btrfs/ioctl.c >>> @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { >>> struct iov_iter iter; >>> }; >>> >>> +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, >>> + unsigned int issue_flags, int rw) >>> +{ >>> + struct btrfs_uring_encoded_data *data = >>> + io_uring_cmd_get_async_data(cmd)->op_data; >>> + int ret; >>> + >>> + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { >>> + data->iov = NULL; >>> + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, >>> + data->args.iovcnt, >>> + ITER_DEST, issue_flags, >>> + &data->iter); >>> + } else { >>> + data->iov = data->iovstack; >>> + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, >>> + ARRAY_SIZE(data->iovstack), &data->iov, >>> + &data->iter); >>> + } >>> + return ret; >>> +} >> >> How can 'cmd' be NULL here? > > It seems that there is no checkes for 'cmd' before and it works same as before. > But I think it's better to add a check in function start for safety. The check goes two lines after you already dereferenced it, and it seems to be called from io_uring cmd specific code. The null check only adds to confusion.
On Tue, Mar 18, 2025 at 07:25:51AM +0000, Pavel Begunkov wrote: > On 3/18/25 01:58, Sidong Yang wrote: > > On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote: > > > On 3/17/25 7:57 AM, Sidong Yang wrote: > > > > This patch introduces btrfs_uring_import_iovec(). In encoded read/write > > > > with uring cmd, it uses import_iovec without supporting fixed buffer. > > > > btrfs_using_import_iovec() could use fixed buffer if cmd flags has > > > > IORING_URING_CMD_FIXED. > > > > > > > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> > > > > --- > > > > fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- > > > > 1 file changed, 24 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > > > index 6c18bad53cd3..a7b52fd99059 100644 > > > > --- a/fs/btrfs/ioctl.c > > > > +++ b/fs/btrfs/ioctl.c > > > > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { > > > > struct iov_iter iter; > > > > }; > > > > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, > > > > + unsigned int issue_flags, int rw) > > > > +{ > > > > + struct btrfs_uring_encoded_data *data = > > > > + io_uring_cmd_get_async_data(cmd)->op_data; > > > > + int ret; > > > > + > > > > + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { > > > > + data->iov = NULL; > > > > + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, > > > > + data->args.iovcnt, > > > > + ITER_DEST, issue_flags, > > > > + &data->iter); > > > > + } else { > > > > + data->iov = data->iovstack; > > > > + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, > > > > + ARRAY_SIZE(data->iovstack), &data->iov, > > > > + &data->iter); > > > > + } > > > > + return ret; > > > > +} > > > > > > How can 'cmd' be NULL here? > > > > It seems that there is no checkes for 'cmd' before and it works same as before. > > But I think it's better to add a check in function start for safety. > > The check goes two lines after you already dereferenced it, and it > seems to be called from io_uring cmd specific code. The null check > only adds to confusion. You mean 'cmd' already dereferenced for async_data. Is it okay to just delete code checking 'cmd' like below? if (cmd->flags & IORING_URING_CMD_FIXED) { Thanks, Sidong > > -- > Pavel Begunkov >
On 3/18/25 1:55 AM, Sidong Yang wrote: > On Tue, Mar 18, 2025 at 07:25:51AM +0000, Pavel Begunkov wrote: >> On 3/18/25 01:58, Sidong Yang wrote: >>> On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote: >>>> On 3/17/25 7:57 AM, Sidong Yang wrote: >>>>> This patch introduces btrfs_uring_import_iovec(). In encoded read/write >>>>> with uring cmd, it uses import_iovec without supporting fixed buffer. >>>>> btrfs_using_import_iovec() could use fixed buffer if cmd flags has >>>>> IORING_URING_CMD_FIXED. >>>>> >>>>> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> >>>>> --- >>>>> fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- >>>>> 1 file changed, 24 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >>>>> index 6c18bad53cd3..a7b52fd99059 100644 >>>>> --- a/fs/btrfs/ioctl.c >>>>> +++ b/fs/btrfs/ioctl.c >>>>> @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { >>>>> struct iov_iter iter; >>>>> }; >>>>> +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, >>>>> + unsigned int issue_flags, int rw) >>>>> +{ >>>>> + struct btrfs_uring_encoded_data *data = >>>>> + io_uring_cmd_get_async_data(cmd)->op_data; >>>>> + int ret; >>>>> + >>>>> + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { >>>>> + data->iov = NULL; >>>>> + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, >>>>> + data->args.iovcnt, >>>>> + ITER_DEST, issue_flags, >>>>> + &data->iter); >>>>> + } else { >>>>> + data->iov = data->iovstack; >>>>> + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, >>>>> + ARRAY_SIZE(data->iovstack), &data->iov, >>>>> + &data->iter); >>>>> + } >>>>> + return ret; >>>>> +} >>>> >>>> How can 'cmd' be NULL here? >>> >>> It seems that there is no checkes for 'cmd' before and it works same as before. >>> But I think it's better to add a check in function start for safety. >> >> The check goes two lines after you already dereferenced it, and it >> seems to be called from io_uring cmd specific code. The null check >> only adds to confusion. > > You mean 'cmd' already dereferenced for async_data. Is it okay to just > delete code checking 'cmd' like below? > > if (cmd->flags & IORING_URING_CMD_FIXED) { Either 'cmd' may be NULL and it's valid (and the current code is fine), or it'd be invalid, in which case that should return an error. But we don't add random pointer == NULL checks as defensive programming. And this one should never ever see cmd == NULL, hence the code needs to go away. It's just confusing otherwise. Consider you reading some code trying to understand what it does, and it has a check for cmd == NULL in there. That would make you wonder "hmm what cases pass in a NULL for cmd here?". When the answer is "this should never happen", don't have the check. It just obfuscates the code.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 6c18bad53cd3..a7b52fd99059 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data { struct iov_iter iter; }; +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd, + unsigned int issue_flags, int rw) +{ + struct btrfs_uring_encoded_data *data = + io_uring_cmd_get_async_data(cmd)->op_data; + int ret; + + if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) { + data->iov = NULL; + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, + data->args.iovcnt, + ITER_DEST, issue_flags, + &data->iter); + } else { + data->iov = data->iovstack; + ret = import_iovec(rw, data->args.iov, data->args.iovcnt, + ARRAY_SIZE(data->iovstack), &data->iov, + &data->iter); + } + return ret; +} + static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags) { size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args, flags); @@ -4874,10 +4896,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue goto out_acct; } - data->iov = data->iovstack; - ret = import_iovec(ITER_DEST, data->args.iov, data->args.iovcnt, - ARRAY_SIZE(data->iovstack), &data->iov, - &data->iter); + ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_DEST); if (ret < 0) goto out_acct; @@ -5022,10 +5041,7 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu if (data->args.len > data->args.unencoded_len - data->args.unencoded_offset) goto out_acct; - data->iov = data->iovstack; - ret = import_iovec(ITER_SOURCE, data->args.iov, data->args.iovcnt, - ARRAY_SIZE(data->iovstack), &data->iov, - &data->iter); + ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_SOURCE); if (ret < 0) goto out_acct;
This patch introduces btrfs_uring_import_iovec(). In encoded read/write with uring cmd, it uses import_iovec without supporting fixed buffer. btrfs_using_import_iovec() could use fixed buffer if cmd flags has IORING_URING_CMD_FIXED. Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> --- fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)