Re: [PATCH v2 4/4] NFSD: handle unaligned DIO for NFS reexport

[Mike Snitzer <snitzer@kernel.org>]

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@kernel.org>
> > > ---
> > >  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.