| Message ID | 20240812161839.1961311-1-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] fuse: Allow page aligned writes | expand |
On Mon, Aug 12, 2024 at 9:37 AM Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > Sorry, I had sent out the wrong/old patch file - it doesn't have one change > (handling of already aligned buffers). > Shall I sent v4? The correct version is below > > --- > > From: Bernd Schubert <bschubert@ddn.com> > Date: Fri, 21 Jun 2024 11:51:23 +0200 > Subject: [PATCH v3] fuse: Allow page aligned writes > > Write IOs should be page aligned as fuse server > might need to copy data to another buffer otherwise in > order to fulfill network or device storage requirements. > > Simple reproducer is with libfuse, example/passthrough* > and opening a file with O_DIRECT - without this change > writing to that file failed with -EINVAL if the underlying > file system was requiring alignment. > > Required server side changes: > Server needs to seek to the next page, without splice that is > just page size buffer alignment, with splice another splice > syscall is needed to seek over the unaligned area. > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > Reviewed-by: Joanne Koong <joannelkoong@gmail.com> > --- > > Changes since v2: > - Added a no-op return in fuse_copy_align for buffers that are > already aligned (cs->len == PAGE_SIZE && cs->offset == 0). Some > server implementations actually do that to compensate for fuse client > misalignment. And it could also happen by accident for non aligned > server allocation. > Added suggestions from Joannes review: > - Removed two sanity checks in fuse_copy_align() to have it > generic. > - Moved from args->in_args[0].align to args->in_args[1].align > to have it in the arg that actually needs the alignment > (for FUSE_WRITE) and updated fuse_copy_args() to align that arg. > - Slight update in the commit body (removed "Reads"). > > libfuse patch: > https://github.com/libfuse/libfuse/pull/983 > > From implmentation point of view it is debatable if request type > parsing should be done in fuse_copy_args() (or fuse_dev_do_read() > or if alignment should be stored in struct fuse_arg / fuse_in_arg. > There are pros and cons for both, I kept it in args as it is > more generic and also allows to later on align other request > types, for example FUSE_SETXATTR. > --- > fs/fuse/dev.c | 29 +++++++++++++++++++++++++++-- > fs/fuse/file.c | 6 ++++++ > fs/fuse/fuse_i.h | 6 ++++-- > include/uapi/linux/fuse.h | 9 ++++++++- > 4 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 9eb191b5c4de..072c7bacc4a7 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -1009,6 +1009,25 @@ static int fuse_copy_one(struct fuse_copy_state *cs, void *val, unsigned size) > return 0; > } > > +/* Align to the next page */ > +static int fuse_copy_align(struct fuse_copy_state *cs) > +{ > + /* > + * This could happen if the userspace buffer is aligned in a way that > + * it compensates fuse headers. > + */ > + if (cs->len == PAGE_SIZE && cs->offset == 0) > + return 0; > + > + if (WARN_ON(cs->len == PAGE_SIZE || cs->offset == 0)) > + return -EIO; > + > + /* Seek to the next page */ > + cs->offset += cs->len; > + cs->len = 0; > + return 0; > +} > + > /* Copy request arguments to/from userspace buffer */ > static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, > unsigned argpages, struct fuse_arg *args, > @@ -1019,10 +1038,16 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, > > for (i = 0; !err && i < numargs; i++) { > struct fuse_arg *arg = &args[i]; > - if (i == numargs - 1 && argpages) > + if (i == numargs - 1 && argpages) { > + if (arg->align) { > + err = fuse_copy_align(cs); > + if (err) > + break; > + } > err = fuse_copy_pages(cs, arg->size, zeroing); > - else > + } else { > err = fuse_copy_one(cs, arg->value, arg->size); > + } > } > return err; > } > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index f39456c65ed7..9783d5809ec3 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1062,6 +1062,12 @@ static void fuse_write_args_fill(struct fuse_io_args *ia, struct fuse_file *ff, > args->in_args[0].size = FUSE_COMPAT_WRITE_IN_SIZE; > else > args->in_args[0].size = sizeof(ia->write.in); > + > + if (ff->open_flags & FOPEN_ALIGNED_WRITES) { > + args->in_args[1].align = 1; > + ia->write.in.write_flags |= FUSE_WRITE_ALIGNED; > + } > + > args->in_args[0].value = &ia->write.in; > args->in_args[1].size = count; > args->out_numargs = 1; > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index f23919610313..1600bd7b1d0d 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -275,13 +275,15 @@ struct fuse_file { > > /** One input argument of a request */ > struct fuse_in_arg { > - unsigned size; > + unsigned int size; > + unsigned int align:1; > const void *value; > }; > > /** One output argument of a request */ > struct fuse_arg { > - unsigned size; > + unsigned int size; > + unsigned int align:1; > void *value; > }; > > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h > index d08b99d60f6f..742262c7c1eb 100644 > --- a/include/uapi/linux/fuse.h > +++ b/include/uapi/linux/fuse.h > @@ -217,6 +217,9 @@ > * - add backing_id to fuse_open_out, add FOPEN_PASSTHROUGH open flag > * - add FUSE_NO_EXPORT_SUPPORT init flag > * - add FUSE_NOTIFY_RESEND, add FUSE_HAS_RESEND init flag > + * > + * 7.41 > + * - add FOPEN_ALIGNED_WRITES open flag and FUSE_WRITE_ALIGNED write flag > */ > > #ifndef _LINUX_FUSE_H > @@ -252,7 +255,7 @@ > #define FUSE_KERNEL_VERSION 7 > > /** Minor version number of this interface */ > -#define FUSE_KERNEL_MINOR_VERSION 40 > +#define FUSE_KERNEL_MINOR_VERSION 41 > > /** The node ID of the root inode */ > #define FUSE_ROOT_ID 1 > @@ -360,6 +363,7 @@ struct fuse_file_lock { > * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE) > * FOPEN_PARALLEL_DIRECT_WRITES: Allow concurrent direct writes on the same inode > * FOPEN_PASSTHROUGH: passthrough read/write io for this open file > + * FOPEN_ALIGNED_WRITES: Page align write io data > */ > #define FOPEN_DIRECT_IO (1 << 0) > #define FOPEN_KEEP_CACHE (1 << 1) > @@ -369,6 +373,7 @@ struct fuse_file_lock { > #define FOPEN_NOFLUSH (1 << 5) > #define FOPEN_PARALLEL_DIRECT_WRITES (1 << 6) > #define FOPEN_PASSTHROUGH (1 << 7) > +#define FOPEN_ALIGNED_WRITES (1 << 8) > > /** > * INIT request/reply flags > @@ -496,10 +501,12 @@ struct fuse_file_lock { > * FUSE_WRITE_CACHE: delayed write from page cache, file handle is guessed > * FUSE_WRITE_LOCKOWNER: lock_owner field is valid > * FUSE_WRITE_KILL_SUIDGID: kill suid and sgid bits > + * FUSE_WRITE_ALIGNED: Write io data are page aligned > */ > #define FUSE_WRITE_CACHE (1 << 0) > #define FUSE_WRITE_LOCKOWNER (1 << 1) > #define FUSE_WRITE_KILL_SUIDGID (1 << 2) > +#define FUSE_WRITE_ALIGNED (1 << 3) > > /* Obsolete alias; this flag implies killing suid/sgid only. */ > #define FUSE_WRITE_KILL_PRIV FUSE_WRITE_KILL_SUIDGID > -- > 2.43.0 > >
On Mon, Aug 12, 2024 at 06:37:49PM +0200, Bernd Schubert wrote: > Sorry, I had sent out the wrong/old patch file - it doesn't have one change > (handling of already aligned buffers). > Shall I sent v4? The correct version is below > > --- > > From: Bernd Schubert <bschubert@ddn.com> > Date: Fri, 21 Jun 2024 11:51:23 +0200 > Subject: [PATCH v3] fuse: Allow page aligned writes > > Write IOs should be page aligned as fuse server > might need to copy data to another buffer otherwise in > order to fulfill network or device storage requirements. > > Simple reproducer is with libfuse, example/passthrough* > and opening a file with O_DIRECT - without this change > writing to that file failed with -EINVAL if the underlying > file system was requiring alignment. > > Required server side changes: > Server needs to seek to the next page, without splice that is > just page size buffer alignment, with splice another splice > syscall is needed to seek over the unaligned area. > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> This looks good, Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks Bernd, Josef
On Mon, 12 Aug 2024 at 18:37, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > Sorry, I had sent out the wrong/old patch file - it doesn't have one change > (handling of already aligned buffers). > Shall I sent v4? The correct version is below > > --- > > From: Bernd Schubert <bschubert@ddn.com> > Date: Fri, 21 Jun 2024 11:51:23 +0200 > Subject: [PATCH v3] fuse: Allow page aligned writes > > Write IOs should be page aligned as fuse server > might need to copy data to another buffer otherwise in > order to fulfill network or device storage requirements. Okay. So why not align the buffer in userspace so the payload of the write request lands on a page boundary? Just the case that you have noted in the fuse_copy_align() function. Thanks, Miklos
On 8/29/24 14:46, Miklos Szeredi wrote: > On Mon, 12 Aug 2024 at 18:37, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: >> >> Sorry, I had sent out the wrong/old patch file - it doesn't have one change >> (handling of already aligned buffers). >> Shall I sent v4? The correct version is below >> >> --- >> >> From: Bernd Schubert <bschubert@ddn.com> >> Date: Fri, 21 Jun 2024 11:51:23 +0200 >> Subject: [PATCH v3] fuse: Allow page aligned writes >> >> Write IOs should be page aligned as fuse server >> might need to copy data to another buffer otherwise in >> order to fulfill network or device storage requirements. > > Okay. > > So why not align the buffer in userspace so the payload of the write > request lands on a page boundary? > > Just the case that you have noted in the fuse_copy_align() function. How would you do that with splice? Thanks, Bernd
On 8/29/24 15:04, Bernd Schubert wrote: > On 8/29/24 14:46, Miklos Szeredi wrote: >> On Mon, 12 Aug 2024 at 18:37, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: >>> >>> Sorry, I had sent out the wrong/old patch file - it doesn't have one change >>> (handling of already aligned buffers). >>> Shall I sent v4? The correct version is below >>> >>> --- >>> >>> From: Bernd Schubert <bschubert@ddn.com> >>> Date: Fri, 21 Jun 2024 11:51:23 +0200 >>> Subject: [PATCH v3] fuse: Allow page aligned writes >>> >>> Write IOs should be page aligned as fuse server >>> might need to copy data to another buffer otherwise in >>> order to fulfill network or device storage requirements. >> >> Okay. >> >> So why not align the buffer in userspace so the payload of the write >> request lands on a page boundary? >> >> Just the case that you have noted in the fuse_copy_align() function. > > How would you do that with splice? > Sorry forgot, additionally we (at DDN) will probably also need aligned setxattr. Thanks, Bernd
On Thu, 29 Aug 2024 at 15:05, Bernd Schubert <bschubert@ddn.com> wrote: > > On 8/29/24 14:46, Miklos Szeredi wrote: > > On Mon, 12 Aug 2024 at 18:37, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > >> > >> Sorry, I had sent out the wrong/old patch file - it doesn't have one change > >> (handling of already aligned buffers). > >> Shall I sent v4? The correct version is below > >> > >> --- > >> > >> From: Bernd Schubert <bschubert@ddn.com> > >> Date: Fri, 21 Jun 2024 11:51:23 +0200 > >> Subject: [PATCH v3] fuse: Allow page aligned writes > >> > >> Write IOs should be page aligned as fuse server > >> might need to copy data to another buffer otherwise in > >> order to fulfill network or device storage requirements. > > > > Okay. > > > > So why not align the buffer in userspace so the payload of the write > > request lands on a page boundary? > > > > Just the case that you have noted in the fuse_copy_align() function. > > How would you do that with splice? I would have thought that splice already honored page boundaries, but maybe commit 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io") broke something there. Should be fixable easily without need to change the interface, because splice is free to start a new buffer at any point. Thanks, Miklos
On Thu, 29 Aug 2024 at 15:07, Bernd Schubert <bschubert@ddn.com> wrote: > Sorry forgot, additionally we (at DDN) will probably also need aligned > setxattr. Hmm, that's trickier. I'm thinking that with the uring interface it would be possible to e.g. give a separate buffer for the header and the payload. But that's just handwaving, would need to flesh out the details. Thanks, Miklos
On 8/29/24 15:45, Miklos Szeredi wrote: > On Thu, 29 Aug 2024 at 15:07, Bernd Schubert <bschubert@ddn.com> wrote: > >> Sorry forgot, additionally we (at DDN) will probably also need aligned >> setxattr. > > Hmm, that's trickier. > > I'm thinking that with the uring interface it would be possible to > e.g. give a separate buffer for the header and the payload. But > that's just handwaving, would need to flesh out the details. New fuse-uring rfc should go out today, but I plan to do another change after that to get the buffer layout closer (or hopefully exactly) to the existing buffer layout to avoid differences. Current uring has fuse_in_header/fuse_out_header separated from the request, but then there is still the per request header. We could change it, also for non-uring, but it would be a rather big change. At least for userspace. My personal preference would be something like #define IN_OUT_HEADER_SZ 128 #define REQ_HEADER_SZ 256 struct fuse_header { union { struct fuse_in_header in; struct fuse_out_header out; uint8_t in_out_header[IN_OUT_HEADER_SZ]; }; union { struct fuse_init_in init_in; struct fuse_init_out init_out; struct cuse_init_in cuse_in; struct cuse_init_out cuse_out; ... uint8_t req_header[REQ_HEADER_SZ]; } } And then actual data would follow after. Would solve changes in struct sizes, up to IN_OUT_HEADER_SZ and REQ_HEADER_SZ, but rather cumbersome. Thanks, Bernd
On 8/29/24 15:39, Miklos Szeredi wrote: > On Thu, 29 Aug 2024 at 15:05, Bernd Schubert <bschubert@ddn.com> wrote: >> >> On 8/29/24 14:46, Miklos Szeredi wrote: >>> On Mon, 12 Aug 2024 at 18:37, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: >>>> >>>> Sorry, I had sent out the wrong/old patch file - it doesn't have one change >>>> (handling of already aligned buffers). >>>> Shall I sent v4? The correct version is below >>>> >>>> --- >>>> >>>> From: Bernd Schubert <bschubert@ddn.com> >>>> Date: Fri, 21 Jun 2024 11:51:23 +0200 >>>> Subject: [PATCH v3] fuse: Allow page aligned writes >>>> >>>> Write IOs should be page aligned as fuse server >>>> might need to copy data to another buffer otherwise in >>>> order to fulfill network or device storage requirements. >>> >>> Okay. >>> >>> So why not align the buffer in userspace so the payload of the write >>> request lands on a page boundary? >>> >>> Just the case that you have noted in the fuse_copy_align() function. >> >> How would you do that with splice? > > I would have thought that splice already honored page boundaries, but > maybe commit 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for > direct_io") broke something there. Should be fixable easily without > need to change the interface, because splice is free to start a new > buffer at any point. Actually my mistake, I had thought that splice has the same issue and I really thought I had tested it, but actually splice works fine and has aligned pages. Thanks, Bernd
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 9eb191b5c4de..139b059f3081 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1009,6 +1009,18 @@ static int fuse_copy_one(struct fuse_copy_state *cs, void *val, unsigned size) return 0; } +/* Align to the next page */ +static int fuse_copy_align(struct fuse_copy_state *cs) +{ + if (WARN_ON(cs->len == PAGE_SIZE || cs->offset == 0)) + return -EIO; + + /* Seek to the next page */ + cs->offset += cs->len; + cs->len = 0; + return 0; +} + /* Copy request arguments to/from userspace buffer */ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, unsigned argpages, struct fuse_arg *args, @@ -1019,10 +1031,16 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, for (i = 0; !err && i < numargs; i++) { struct fuse_arg *arg = &args[i]; - if (i == numargs - 1 && argpages) + if (i == numargs - 1 && argpages) { + if (arg->align) { + err = fuse_copy_align(cs); + if (err) + break; + } err = fuse_copy_pages(cs, arg->size, zeroing); - else + } else { err = fuse_copy_one(cs, arg->value, arg->size); + } } return err; } diff --git a/fs/fuse/file.c b/fs/fuse/file.c index f39456c65ed7..9783d5809ec3 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1062,6 +1062,12 @@ static void fuse_write_args_fill(struct fuse_io_args *ia, struct fuse_file *ff, args->in_args[0].size = FUSE_COMPAT_WRITE_IN_SIZE; else args->in_args[0].size = sizeof(ia->write.in); + + if (ff->open_flags & FOPEN_ALIGNED_WRITES) { + args->in_args[1].align = 1; + ia->write.in.write_flags |= FUSE_WRITE_ALIGNED; + } + args->in_args[0].value = &ia->write.in; args->in_args[1].size = count; args->out_numargs = 1; diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index f23919610313..1600bd7b1d0d 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -275,13 +275,15 @@ struct fuse_file { /** One input argument of a request */ struct fuse_in_arg { - unsigned size; + unsigned int size; + unsigned int align:1; const void *value; }; /** One output argument of a request */ struct fuse_arg { - unsigned size; + unsigned int size; + unsigned int align:1; void *value; }; diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index d08b99d60f6f..742262c7c1eb 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -217,6 +217,9 @@ * - add backing_id to fuse_open_out, add FOPEN_PASSTHROUGH open flag * - add FUSE_NO_EXPORT_SUPPORT init flag * - add FUSE_NOTIFY_RESEND, add FUSE_HAS_RESEND init flag + * + * 7.41 + * - add FOPEN_ALIGNED_WRITES open flag and FUSE_WRITE_ALIGNED write flag */ #ifndef _LINUX_FUSE_H @@ -252,7 +255,7 @@ #define FUSE_KERNEL_VERSION 7 /** Minor version number of this interface */ -#define FUSE_KERNEL_MINOR_VERSION 40 +#define FUSE_KERNEL_MINOR_VERSION 41 /** The node ID of the root inode */ #define FUSE_ROOT_ID 1 @@ -360,6 +363,7 @@ struct fuse_file_lock { * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE) * FOPEN_PARALLEL_DIRECT_WRITES: Allow concurrent direct writes on the same inode * FOPEN_PASSTHROUGH: passthrough read/write io for this open file + * FOPEN_ALIGNED_WRITES: Page align write io data */ #define FOPEN_DIRECT_IO (1 << 0) #define FOPEN_KEEP_CACHE (1 << 1) @@ -369,6 +373,7 @@ struct fuse_file_lock { #define FOPEN_NOFLUSH (1 << 5) #define FOPEN_PARALLEL_DIRECT_WRITES (1 << 6) #define FOPEN_PASSTHROUGH (1 << 7) +#define FOPEN_ALIGNED_WRITES (1 << 8) /** * INIT request/reply flags @@ -496,10 +501,12 @@ struct fuse_file_lock { * FUSE_WRITE_CACHE: delayed write from page cache, file handle is guessed * FUSE_WRITE_LOCKOWNER: lock_owner field is valid * FUSE_WRITE_KILL_SUIDGID: kill suid and sgid bits + * FUSE_WRITE_ALIGNED: Write io data are page aligned */ #define FUSE_WRITE_CACHE (1 << 0) #define FUSE_WRITE_LOCKOWNER (1 << 1) #define FUSE_WRITE_KILL_SUIDGID (1 << 2) +#define FUSE_WRITE_ALIGNED (1 << 3) /* Obsolete alias; this flag implies killing suid/sgid only. */ #define FUSE_WRITE_KILL_PRIV FUSE_WRITE_KILL_SUIDGID
Write IOs should be page aligned as fuse server might need to copy data to another buffer otherwise in order to fulfill network or device storage requirements. Simple reproducer is with libfuse, example/passthrough* and opening a file with O_DIRECT - without this change writing to that file failed with -EINVAL if the underlying file system was requiring alignment. Required server side changes: Server needs to seek to the next page, without splice that is just page size buffer alignment, with splice another splice syscall is needed to seek over the unaligned area. Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- Changes since v2: - Added a no-op return in fuse_copy_align for buffers that are already aligned (cs->len == PAGE_SIZE && cs->offset == 0). Some server implementations actually do that to compensate for fuse client misalignment. And it could also happen by accident for non aligned server allocation. Added suggestions from Joannes review: - Removed two sanity checks in fuse_copy_align() to have it generic. - Moved from args->in_args[0].align to args->in_args[1].align to have it in the arg that actually needs the alignment (for FUSE_WRITE) and updated fuse_copy_args() to align that arg. - Slight update in the commit body (removed "Reads"). libfuse patch: https://github.com/libfuse/libfuse/pull/983 From implmentation point of view it is debatable if request type parsing should be done in fuse_copy_args() (or fuse_dev_do_read() or if alignment should be stored in struct fuse_arg / fuse_in_arg. There are pros and cons for both, I kept it in args as it is more generic and also allows to later on align other request types, for example FUSE_SETXATTR. --- fs/fuse/dev.c | 22 ++++++++++++++++++++-- fs/fuse/file.c | 6 ++++++ fs/fuse/fuse_i.h | 6 ++++-- include/uapi/linux/fuse.h | 9 ++++++++- 4 files changed, 38 insertions(+), 5 deletions(-)