On Thu, Jul 31, 2025 at 05:28:13PM -0400, Mike Snitzer wrote:
> On Thu, Jul 31, 2025 at 04:58:00PM -0400, Jeff Layton wrote:
> > On Thu, 2025-07-31 at 15:44 -0400, Mike Snitzer wrote:
> > > NFS doesn't have any DIO alignment constraints but it doesn't support
> > > STATX_DIOALIGN, so update NFSD such that it doesn't disable the use of
> > > NFSD_IO_DIRECT if it is reexporting NFS.
> > >
> > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > > ---
> > > fs/nfs/export.c | 3 ++-
> > > fs/nfsd/filecache.c | 11 +++++++++++
> > > include/linux/exportfs.h | 13 +++++++++++++
> > > 3 files changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfs/export.c b/fs/nfs/export.c
> > > index e9c233b6fd209..2cae75ba6b35d 100644
> > > --- a/fs/nfs/export.c
> > > +++ b/fs/nfs/export.c
> > > @@ -155,5 +155,6 @@ const struct export_operations nfs_export_ops = {
> > > EXPORT_OP_REMOTE_FS |
> > > EXPORT_OP_NOATOMIC_ATTR |
> > > EXPORT_OP_FLUSH_ON_CLOSE |
> > > - EXPORT_OP_NOLOCKS,
> > > + EXPORT_OP_NOLOCKS |
> > > + EXPORT_OP_NO_DIOALIGN_NEEDED,
> > > };
> > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > index 5601e839a72da..ea489dd44fd9a 100644
> > > --- a/fs/nfsd/filecache.c
> > > +++ b/fs/nfsd/filecache.c
> > > @@ -1066,6 +1066,17 @@ nfsd_file_getattr(const struct svc_fh *fhp, struct nfsd_file *nf)
> > > nfsd_io_cache_write != NFSD_IO_DIRECT))
> > > return nfs_ok;
> > >
> > > + if (exportfs_handles_unaligned_dio(nf->nf_file->f_path.mnt->mnt_sb->s_export_op)) {
> > > + /* Underlying filesystem doesn't support STATX_DIOALIGN
> > > + * but it can handle all unaligned DIO, so establish
> > > + * DIO alignment that is accommodating.
> > > + */
> > > + nf->nf_dio_mem_align = 4;
> > > + nf->nf_dio_offset_align = PAGE_SIZE;
> > > + nf->nf_dio_read_offset_align = nf->nf_dio_offset_align;
> > > + return nfs_ok;
> > > + }
> > > +
> > > status = fh_getattr(fhp, &stat);
> > > if (status != nfs_ok)
> > > return status;
> > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > index 9369a607224c1..626b8486dd985 100644
> > > --- a/include/linux/exportfs.h
> > > +++ b/include/linux/exportfs.h
> > > @@ -247,6 +247,7 @@ struct export_operations {
> > > */
> > > #define EXPORT_OP_FLUSH_ON_CLOSE (0x20) /* fs flushes file data on close */
> > > #define EXPORT_OP_NOLOCKS (0x40) /* no file locking support */
> > > +#define EXPORT_OP_NO_DIOALIGN_NEEDED (0x80) /* fs can handle unaligned DIO */
> > > unsigned long flags;
> > > };
> > >
> > > @@ -262,6 +263,18 @@ exportfs_cannot_lock(const struct export_operations *export_ops)
> > > return export_ops->flags & EXPORT_OP_NOLOCKS;
> > > }
> > >
> > > +/**
> > > + * exportfs_handles_unaligned_dio() - check if export can handle unaligned DIO
> > > + * @export_ops: the nfs export operations to check
> > > + *
> > > + * Returns true if the export can handle unaligned DIO.
> > > + */
> > > +static inline bool
> > > +exportfs_handles_unaligned_dio(const struct export_operations *export_ops)
> > > +{
> > > + return export_ops->flags & EXPORT_OP_NO_DIOALIGN_NEEDED;
> > > +}
> > > +
> > > extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> > > int *max_len, struct inode *parent,
> > > int flags);
> >
> >
> > Would it not be simpler (better?) to add support for STATX_DIOALIGN to
> > NFS, and just have it report '1' for both values?
>
> I suppose adding NFS support for STATX_DIOALIGN, that doesn't actually
> go over the wire, does make sense.
>
> But I wouldn't think setting them to 1 valid. Pretty sure they need
> to be a power-of-2 (since they are used as masks passed to
> iov_iter_is_aligned).
>
> In addition, we want to make sure NFS's default DIO alignment (which
> isn't informed by actual DIO alignment advertised by NFSD's underlying
> filesystem and hardware, e.g. XFS and NVMe) is able to be compatible
> with both finer (512b) and coarser (4096b) grained DIO alignment.
> Only way to achieve that would be to skew toward the coarse-grained
> end of the spectrum, right?
>
> More conservative but more likely to work with everything.
Thinking/looking further: I really do prefer the approach I took in
this patch (over the suggestion to implement STATX_DIOALIGN for NFS).
Otherwise NFS would forced to needlessly issue an RPC via fh_getattr()
even though we're talking about NFS faking its STATX_DIOALIGN response
(so it doesn't need to do the work to do a full-blown GETATTR).
This would be wasteful for the NFS reexport usecase.
