On Tue, Sep 2, 2025 at 3:58 PM Haihua Yang <yanghh@xxxxxxxxx> wrote: > > Trond, > Thanks for reviewing the change. I understand that, logically, if a > WRITE is FILE_SYNC, a LAYOUTCOMMIT isn’t necessary. I’m working on > enabling file_layout on some legacy POSIX filesystems, where > synchronizing metadata between the MDS and DS during client writes to > the DS incurs significant overhead. To address this, I’m suggesting an > option that allows the MDS to update the metadata. From my reading, > RFC8881 doesn’t seem to prohibit LAYOUTCOMMIT in this scenario, could > you share more detail about how it would break the FILE_SYNC case? If you look at this email thread that was on nfsv4@xxxxxxxx in 2019, you'll see some discussion of this. I do not believe there was an agreement that the client do a LAYOUTCOMMIT for this case. (You'll see I discovered what I believe you have discovered.) https://mailarchive.ietf.org/arch/msg/nfsv4/eQNilayKCMA43RbzjjuttjdBZ7Q/ rick > Thanks > Haihua > > On Tue, Sep 2, 2025 at 2:18 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > > > On Tue, 2025-09-02 at 18:19 +0000, Haihua Yang wrote: > > > [You don't often get email from yanghh@xxxxxxxxx. Learn why this is > > > important at https://aka.ms/LearnAboutSenderIdentification ] > > > > > > --- > > > fs/nfs/filelayout/filelayout.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/nfs/filelayout/filelayout.c > > > b/fs/nfs/filelayout/filelayout.c > > > index d39a1f58e18d..1cb8f413a665 100644 > > > --- a/fs/nfs/filelayout/filelayout.c > > > +++ b/fs/nfs/filelayout/filelayout.c > > > @@ -48,6 +48,8 @@ MODULE_LICENSE("GPL"); > > > MODULE_AUTHOR("Dean Hildebrand <dhildebz@xxxxxxxxx>"); > > > MODULE_DESCRIPTION("The NFSv4 file layout driver"); > > > > > > +static bool force_layoutcommit = false; > > > + > > > #define FILELAYOUT_POLL_RETRY_MAX (15*HZ) > > > static const struct pnfs_commit_ops filelayout_commit_ops; > > > > > > @@ -233,10 +235,11 @@ filelayout_set_layoutcommit(struct > > > nfs_pgio_header *hdr) > > > { > > > loff_t end_offs = 0; > > > > > > - if (FILELAYOUT_LSEG(hdr->lseg)->commit_through_mds || > > > - hdr->res.verf->committed == NFS_FILE_SYNC) > > > + if (!force_layoutcommit && (FILELAYOUT_LSEG(hdr->lseg)- > > > >commit_through_mds || > > > + hdr->res.verf->committed == NFS_FILE_SYNC)) > > > return; > > > - if (hdr->res.verf->committed == NFS_DATA_SYNC) > > > + if (hdr->res.verf->committed == NFS_DATA_SYNC || > > > + (force_layoutcommit && hdr->res.verf->committed == > > > NFS_FILE_SYNC)) > > > end_offs = hdr->mds_offset + (loff_t)hdr->res.count; > > > > > > /* Note: if the write is unstable, don't set end_offs until > > > commit */ > > > @@ -1149,5 +1152,8 @@ static void __exit nfs4filelayout_exit(void) > > > > > > MODULE_ALIAS("nfs-layouttype4-1"); > > > > > > +module_param(force_layoutcommit, bool, 0644); > > > +MODULE_PARM_DESC(force_layoutcommit, "Force LAYOUTCOMMIT after data > > > was written (default: false)"); > > > + > > > module_init(nfs4filelayout_init); > > > module_exit(nfs4filelayout_exit); > > > -- > > > 2.51.0.87.g1fa68948c3.dirty > > > > > > > NACK. > > > > Firstly, there is no documentation that tells either me or any > > pnfs/files user when it would be desirable to set this flag. There > > isn't even a reference to what problem in RFC8881 it addresses. > > > > Secondly, it breaks the NFS_FILE_SYNC case. > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trondmy@xxxxxxxxxx, trond.myklebust@xxxxxxxxxxxxxxx >
