Hi Bruce-
During some testing I noticed that OPEN frequently returns a
zero in the cinfo.after field on my test share, which is tmpfs.
Does not seem to be an issue for xfs.
For xfs, we take the 2nd arm in nfsd4_change_attribute(), and
for tmpfs, we take the 3rd arm:
309 static inline u64 nfsd4_change_attribute(struct kstat *stat,
310 struct inode *inode)
311 {
312 if (inode->i_sb->s_export_op->fetch_iversion)
313 return inode->i_sb->s_export_op->fetch_iversion(inode);
314 else if (IS_I_VERSION(inode)) {
315 u64 chattr;
316
317 chattr = stat->ctime.tv_sec;
318 chattr <<= 30;
319 chattr += stat->ctime.tv_nsec;
320 chattr += inode_query_iversion(inode);
321 return chattr;
322 } else
323 return time_to_chattr(&stat->ctime);
324 }
Thus for tmpfs, ->fetch_iversion() is NULL and IS_I_VERSION is false.
Since commit 428a23d2bf0c ("nfsd: skip some unnecessary stats in
the v4 case"), fill_post_wcc() looks like this:
518 static bool fs_supports_change_attribute(struct super_block *sb)
519 {
520 return sb->s_flags & SB_I_VERSION || sb->s_export_op->fetch_iversion;
521 }
...
557 void fill_post_wcc(struct svc_fh *fhp)
558 {
559 bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
560 struct inode *inode = d_inode(fhp->fh_dentry);
561
562 if (fhp->fh_no_wcc)
563 return;
564
565 if (fhp->fh_post_saved)
566 printk("nfsd: inode locked twice during operation.\n");
567
568 fhp->fh_post_saved = true;
569
570 if (fs_supports_change_attribute(inode->i_sb) || !v4) {
571 __be32 err = fh_getattr(fhp, &fhp->fh_post_attr);
572
573 if (err) {
574 fhp->fh_post_saved = false;
575 fhp->fh_post_attr.ctime = inode->i_ctime;
576 }
577 }
578 if (v4)
579 fhp->fh_post_change =
580 nfsd4_change_attribute(&fhp->fh_post_attr, inode);
581 }
fs_support_change_attribute() returns false for tmpfs, and !v4
evaluates to false for OPEN operations. That means the fs_getattr()
is never invoked and nfsd4_change_attribute() is called with an
uninitialized fh_post_attr.
It appears that the same problem exists in fill_pre_wcc() but the
symptoms are different. This is because for tmpfs, the local variable
@stat is not initialized -- it contains whatever was on the stack
before fill_pre_wcc() was invoked. In other words, the on-the-wire
cinfo.before field contains old stack contents.
So both OPEN result cinfo fields are junk on tmpfs exports.
I haven't checked if "fs_supports_change_attribute(inode->i_sb) || !v4"
happens to evaluate to false for other filesystem types.
An easy way to address this would be to revert 428a23d2bf0c. But I
wonder if there are any particular regression tests in the pynfs
suite that could detect this kind of misbehavior, in case someone
would like to try to re-implement the optimization in 428a23d2bf0c.
--
Chuck Lever
On Fri, Dec 24, 2021 at 04:46:26AM +0000, Chuck Lever III wrote:
> During some testing I noticed that OPEN frequently returns a
> zero in the cinfo.after field on my test share, which is tmpfs.
> Does not seem to be an issue for xfs.
Thanks for catching this.
> An easy way to address this would be to revert 428a23d2bf0c. But I
> wonder if there are any particular regression tests in the pynfs
> suite that could detect this kind of misbehavior, in case someone
> would like to try to re-implement the optimization in 428a23d2bf0c.
From a quick grep the only tests I see checking cinfo are in
nfs4.0/servertests/st_rename.py
--b.