> 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" ? >> >> >>> + * change values, after throwing a warning. >>> + */ >>> + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { >> >> Maybe you should clear ->atomic as well in this case. >> >> >>> + 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
