On Thu, 2023-07-20 at 15:37 +0000, Chuck Lever III wrote: > > > On Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > On Thu, 2023-07-20 at 15:15 +0000, Chuck Lever III wrote: > > > > > > > On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > > > > > At one time, nfsd would scrape inode information directly out of struct > > > > inode in order to populate the change_info4. At that time, the BUG_ON in > > > > set_change_info made some sense, since having it unset meant a coding > > > > error. > > > > > > > > More recently, it calls vfs_getattr to get this information, which can > > > > fail. If that fails, fh_pre_saved can end up not being set. While this > > > > situation is unfortunate, we don't need to crash the box. > > > > > > I'm always happy to get rid of a BUG_ON(). But I'm not sure even > > > a warning is necessary in this case. It's not likely that it's > > > a software bug or something that the server administrator can > > > do something about. > > > > > > Can you elaborate on why the vfs_getattr() might fail? Eg, how > > > was it failing in 2223560 ? > > > > > > > I'm fine with dropping the WARN_ON. You are correct that there is > > probably little the admin can do about it. > > > > vfs_getattr can fail for all sorts of reasons. It really depends on the > > underlying filesystem. In 2223560, I don't know for sure, but just prior > > to the oops, there were these messages in the log: > > > > [51935.482019] XFS (vda3): Filesystem has been shut down due to log error (0x2). > > [51935.482020] XFS (vda3): Please unmount the filesystem and rectify the problem(s). > > [51935.482550] vda3: writeback error on inode 25320400, offset 2097152, sector 58684120 > > > > My assumption was that the fs being shut down caused some VFS operations > > to start returning errors (including getattr) and that is why > > fh_pre_saved ultimately didn't get set. > > I'm wondering if the operation should just fail in this case > rather than return a cobbled-up changeinfo4. Maybe for another > day. > > > > > > Move set_change_info to nfs4proc.c since all of the callers are there. > > > > Revise the condition for setting "atomic" to also check for > > > > fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it > > > > zero out both change_attr4s when this occurs. > > > > > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2223560 > > > > Reported-by: Boyang Xue <bxue@xxxxxxxxxx> > > > > > > checkpatch now wants > > > > > > Reported-by: > > > Closes: > > > > > > in that order. > > > > > > > > > Mmmmkay. So I assume the URL should go in the Closes: field then? > > Yes, a bug link goes in the Closes: field. > > > > I'll take out the WARN_ON_ONCE and resend, once others have had a chance > > to comment. > > Don't miss the other comments below. > > > > Thanks! > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > > > --- > > > > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++ > > > > fs/nfsd/xdr4.h | 11 ----------- > > > > 2 files changed, 30 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > > > index d8e7a533f9d2..e6f406f27821 100644 > > > > --- a/fs/nfsd/nfs4proc.c > > > > +++ b/fs/nfsd/nfs4proc.c > > > > @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > return status; > > > > } > > > > > > > > +/** > > > > + * set_change_info - set up the change_info4 for a reply > > > > + * @cinfo: pointer to nfsd4_change_info to be populated > > > > + * @fhp: pointer to svc_fh to use as source > > > > + * > > > > + * Many operations in NFSv4 require change_info4 in the reply. This function > > > > + * populates that from the info that we (should!) have already collected. In > > > > + * the event that we didn't get any pre-attrs, throw a warning and just > > > > + * zero out both change_attr4 fields. > > > > + */ > > > > +static void > > > > +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > > > +{ > > > > + cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > > > + > > > > + /* > > > > + * In the event that we couldn't fetch attributes from the > > > > + * server for some reason, just zero out the before and after > > > > > > "From the server"? Is it only likely to fail if the exported > > > filesystem is an NFS mount? Or did you mean "from the filesystem" ? > > > Yep, will fix. > > > > > > > + * change values, after throwing a warning. > > > > + */ > > > > + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { > > > > > > Maybe you should clear ->atomic as well in this case. > > > > > > It should already be clear since fh_pre_saved isn't set. > > > > + cinfo->before_change = 0; > > > > + cinfo->after_change = 0; > > > > + return; > > > > + } > > > > + > > > > + cinfo->before_change = fhp->fh_pre_change; > > > > + cinfo->after_change = fhp->fh_post_change; > > > > +} > > > > + > > > > static __be32 > > > > do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) > > > > { > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > > index b2931fdf53be..9e67f63c5f4d 100644 > > > > --- a/fs/nfsd/xdr4.h > > > > +++ b/fs/nfsd/xdr4.h > > > > @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op); > > > > > > > > #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs) > > > > > > > > -static inline void > > > > -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > > > -{ > > > > - BUG_ON(!fhp->fh_pre_saved); > > > > - cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > > > - > > > > - cinfo->before_change = fhp->fh_pre_change; > > > > - cinfo->after_change = fhp->fh_post_change; > > > > -} > > > > - > > > > - > > > > bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp); > > > > bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > > > bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > > > > > > > --- > > > > base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e > > > > change-id: 20230720-bz2223560-9c4690a8217b > > > > > > > > Best regards, > > > > -- > > > > Jeff Layton <jlayton@xxxxxxxxxx> > > > > > > > > > > -- > > > Chuck Lever > > > > > > > > > > -- > > Jeff Layton <jlayton@xxxxxxxxxx> > > > -- > Chuck Lever > > -- Jeff Layton <jlayton@xxxxxxxxxx>
