| Message ID | 20250319061251.21452-6-sidong.yang@furiosa.ai (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | introduce io_uring_cmd_import_fixed_vec | expand |
On 3/19/25 06:12, 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. Looks fine to me. The only comment, it appears btrfs silently ignored IORING_URING_CMD_FIXED before, so theoretically it changes the uapi. It should be fine, but maybe we should sneak in and backport a patch refusing the flag for older kernels? Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> > --- > fs/btrfs/ioctl.c | 34 +++++++++++++++++++++++++--------- > 1 file changed, 25 insertions(+), 9 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 6c18bad53cd3..e5b4af41ca6b 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4802,7 +4802,29 @@ struct btrfs_uring_encoded_data { > struct iov_iter iter; > }; > > -static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags) > +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->flags & IORING_URING_CMD_FIXED) { > + data->iov = NULL; > + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, > + data->args.iovcnt, rw, > + &data->iter, issue_flags); > + } 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); > size_t copy_end; > @@ -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; >
On Thu, Mar 20, 2025 at 12:01:42PM +0000, Pavel Begunkov wrote: > On 3/19/25 06:12, 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. > > Looks fine to me. The only comment, it appears btrfs silently ignored > IORING_URING_CMD_FIXED before, so theoretically it changes the uapi. > It should be fine, but maybe we should sneak in and backport a patch > refusing the flag for older kernels? I think it's okay to leave the old version as it is. Making it to refuse the flag could break user application. Thanks, Sidong > > Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> > > > > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> > > --- > > fs/btrfs/ioctl.c | 34 +++++++++++++++++++++++++--------- > > 1 file changed, 25 insertions(+), 9 deletions(-) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index 6c18bad53cd3..e5b4af41ca6b 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -4802,7 +4802,29 @@ struct btrfs_uring_encoded_data { > > struct iov_iter iter; > > }; > > -static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags) > > +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->flags & IORING_URING_CMD_FIXED) { > > + data->iov = NULL; > > + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, > > + data->args.iovcnt, rw, > > + &data->iter, issue_flags); > > + } 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); > > size_t copy_end; > > @@ -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; > > -- > Pavel Begunkov >
On 3/20/25 16:19, Sidong Yang wrote: > On Thu, Mar 20, 2025 at 12:01:42PM +0000, Pavel Begunkov wrote: >> On 3/19/25 06:12, 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. >> >> Looks fine to me. The only comment, it appears btrfs silently ignored >> IORING_URING_CMD_FIXED before, so theoretically it changes the uapi. >> It should be fine, but maybe we should sneak in and backport a patch >> refusing the flag for older kernels? > > I think it's okay to leave the old version as it is. Making it to refuse > the flag could break user application. Just as this patch breaks it. The cmd is new and quite specific, likely nobody would notice the change. As it currently stands, the fixed buffer version of the cmd is going to succeed in 99% of cases on older kernels because we're still passing an iovec in, but that's only until someone plays remapping games after a registration and gets bizarre results. It's up to btrfs folks how they want to handle that, either try to fix it now, or have a chance someone will be surprised in the future. My recommendation would be the former one.
On 3/21/25 4:28 AM, Pavel Begunkov wrote: > On 3/20/25 16:19, Sidong Yang wrote: >> On Thu, Mar 20, 2025 at 12:01:42PM +0000, Pavel Begunkov wrote: >>> On 3/19/25 06:12, 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. >>> >>> Looks fine to me. The only comment, it appears btrfs silently ignored >>> IORING_URING_CMD_FIXED before, so theoretically it changes the uapi. >>> It should be fine, but maybe we should sneak in and backport a patch >>> refusing the flag for older kernels? >> >> I think it's okay to leave the old version as it is. Making it to refuse >> the flag could break user application. > > Just as this patch breaks it. The cmd is new and quite specific, likely > nobody would notice the change. As it currently stands, the fixed buffer > version of the cmd is going to succeed in 99% of cases on older kernels > because we're still passing an iovec in, but that's only until someone > plays remapping games after a registration and gets bizarre results. > > It's up to btrfs folks how they want to handle that, either try to fix > it now, or have a chance someone will be surprised in the future. My > recommendation would be the former one. I'd strongly recommend that the btrfs side check for valid flags and error it. It's a new enough addition that this should not be a concern, and silently ignoring (currently) unsupported flags rather than erroring them is a mistake. Sidong, please do send a patch for that so it can go into 6.13 stable and 6.14 to avoid any confusion in this area in the future.
On Fri, Mar 21, 2025 at 05:17:15AM -0600, Jens Axboe wrote: > On 3/21/25 4:28 AM, Pavel Begunkov wrote: > > On 3/20/25 16:19, Sidong Yang wrote: > >> On Thu, Mar 20, 2025 at 12:01:42PM +0000, Pavel Begunkov wrote: > >>> On 3/19/25 06:12, 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. > >>> > >>> Looks fine to me. The only comment, it appears btrfs silently ignored > >>> IORING_URING_CMD_FIXED before, so theoretically it changes the uapi. > >>> It should be fine, but maybe we should sneak in and backport a patch > >>> refusing the flag for older kernels? > >> > >> I think it's okay to leave the old version as it is. Making it to refuse > >> the flag could break user application. > > > > Just as this patch breaks it. The cmd is new and quite specific, likely > > nobody would notice the change. As it currently stands, the fixed buffer > > version of the cmd is going to succeed in 99% of cases on older kernels > > because we're still passing an iovec in, but that's only until someone > > plays remapping games after a registration and gets bizarre results. > > > > It's up to btrfs folks how they want to handle that, either try to fix > > it now, or have a chance someone will be surprised in the future. My > > recommendation would be the former one. > > I'd strongly recommend that the btrfs side check for valid flags and > error it. It's a new enough addition that this should not be a concern, > and silently ignoring (currently) unsupported flags rather than erroring > them is a mistake. > > Sidong, please do send a patch for that so it can go into 6.13 stable > and 6.14 to avoid any confusion in this area in the future. Agreed, It could be seen as a bug that the flag is dismissed silently. I'll write a patch for this. Thanks, Sidong > > -- > Jens Axboe
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 6c18bad53cd3..e5b4af41ca6b 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4802,7 +4802,29 @@ struct btrfs_uring_encoded_data { struct iov_iter iter; }; -static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags) +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->flags & IORING_URING_CMD_FIXED) { + data->iov = NULL; + ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov, + data->args.iovcnt, rw, + &data->iter, issue_flags); + } 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); size_t copy_end; @@ -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 | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)