On Sat, 8 Aug 2015 08:27:47 +0800
Kinglong Mee <kinglongmee@xxxxxxxxx> wrote:
> On 8/8/2015 02:24, Jeff Layton wrote:
> > On Fri, 7 Aug 2015 13:56:10 -0400
> > Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote:
> >
> >> On Fri, 7 Aug 2015 23:33:58 +0800
> >> Kinglong Mee <kinglongmee@xxxxxxxxx> wrote:
> >>
> >>> On 8/6/2015 05:13, Jeff Layton wrote:
> >>>> In a later patch, we'll need to be able to cook up a knfsd_fh from a
> >>>> dentry/export combo. Usually nfsd works with svc_fh's, but that takes
> >>>> a lot of extra baggage that we don't really need here.
> >>>>
> >>>> Add a routine that encodes the filehandle directly into a knfsd_fh
> >>>> instead. Much like we have a fh_copy_shallow, the new routine is called
> >>>> fh_compose_shallow.
> >>>>
> >>>> Signed-off-by: Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
> >>>> ---
> >>>> fs/nfsd/nfsfh.c | 112 +++++++++++++++++++++++++++++---------------------------
> >>>> fs/nfsd/nfsfh.h | 2 +
> >>>> 2 files changed, 60 insertions(+), 54 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> >>>> index b601f291d825..ee6d4b746467 100644
> >>>> --- a/fs/nfsd/nfsfh.c
> >>>> +++ b/fs/nfsd/nfsfh.c
> >>>> @@ -511,32 +511,64 @@ retry:
> >>>> }
> >>>>
> >>>> __be32
> >>>> -fh_compose(struct svc_fh *fhp, struct svc_export *exp, struct dentry *dentry,
> >>>> - struct svc_fh *ref_fh)
> >>>> +fh_compose_shallow(struct knfsd_fh *kfh, int maxsize, struct svc_export *exp,
> >>>> + struct dentry *dentry, struct svc_fh *ref_fh)
> >>>> {
> >>>> - /* ref_fh is a reference file handle.
> >>>> - * if it is non-null and for the same filesystem, then we should compose
> >>>> - * a filehandle which is of the same version, where possible.
> >>>> - * Currently, that means that if ref_fh->fh_handle.fh_version == 0xca
> >>>> - * Then create a 32byte filehandle using nfs_fhbase_old
> >>>> - *
> >>>> - */
> >>>> + struct inode *inode = d_inode(dentry);
> >>>> + dev_t ex_dev = exp_sb(exp)->s_dev;
> >>>>
> >>>> - struct inode * inode = d_inode(dentry);
> >>>> - dev_t ex_dev = exp_sb(exp)->s_dev;
> >>>> -
> >>>> - dprintk("nfsd: fh_compose(exp %02x:%02x/%ld %pd2, ino=%ld)\n",
> >>>> - MAJOR(ex_dev), MINOR(ex_dev),
> >>>> + dprintk("nfsd: %s(exp %02x:%02x/%ld %pd2, ino=%ld)\n",
> >>>> + __func__, MAJOR(ex_dev), MINOR(ex_dev),
> >>>> (long) d_inode(exp->ex_path.dentry)->i_ino,
> >>>> - dentry,
> >>>> - (inode ? inode->i_ino : 0));
> >>>> + dentry, (inode ? inode->i_ino : 0));
> >>>>
> >>>> - /* Choose filehandle version and fsid type based on
> >>>> - * the reference filehandle (if it is in the same export)
> >>>> - * or the export options.
> >>>> + /*
> >>>> + * Choose filehandle version and fsid type based on the reference
> >>>> + * filehandle (if it is in the same export) or the export options.
> >>>> */
> >>>> - set_version_and_fsid_type(&fhp->fh_handle, fhp->fh_maxsize,
> >>>> - exp, ref_fh);
> >>>> + set_version_and_fsid_type(kfh, maxsize, exp, ref_fh);
> >>>> + if (kfh->fh_version == 0xca) {
> >>>> + /* old style filehandle please */
> >>>> + memset(&kfh->fh_base, 0, NFS_FHSIZE);
> >>>> + kfh->fh_size = NFS_FHSIZE;
> >>>> + kfh->ofh_dcookie = 0xfeebbaca;
> >>>> + kfh->ofh_dev = old_encode_dev(ex_dev);
> >>>> + kfh->ofh_xdev = kfh->ofh_dev;
> >>>> + kfh->ofh_xino =
> >>>> + ino_t_to_u32(d_inode(exp->ex_path.dentry)->i_ino);
> >>>> + kfh->ofh_dirino = ino_t_to_u32(parent_ino(dentry));
> >>>> + if (inode)
> >>>> + _fh_update_old(dentry, exp, kfh);
> >>>> + } else {
> >>>> + kfh->fh_size = key_len(kfh->fh_fsid_type) + 4;
> >>>> + kfh->fh_auth_type = 0;
> >>>> +
> >>>> + mk_fsid(kfh->fh_fsid_type, kfh->fh_fsid, ex_dev,
> >>>> + d_inode(exp->ex_path.dentry)->i_ino,
> >>>> + exp->ex_fsid, exp->ex_uuid);
> >>>> +
> >>>> + if (inode)
> >>>> + _fh_update(kfh, maxsize, exp, dentry);
> >>>> +
> >>>> + if (kfh->fh_fileid_type == FILEID_INVALID)
> >>>> + return nfserr_opnotsupp;
> >>>> + }
> >>>> + return nfs_ok;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * ref_fh is a reference file handle.
> >>>> + * if it is non-null and for the same filesystem, then we should compose
> >>>> + * a filehandle which is of the same version, where possible.
> >>>> + * Currently, that means that if ref_fh->fh_handle.fh_version == 0xca
> >>>> + * Then create a 32byte filehandle using nfs_fhbase_old
> >>>> + *
> >>>> + */
> >>>> +__be32
> >>>> +fh_compose(struct svc_fh *fhp, struct svc_export *exp, struct dentry *dentry,
> >>>> + struct svc_fh *ref_fh)
> >>>> +{
> >>>> + __be32 status;
> >>>>
> >>>> if (ref_fh == fhp)
> >>>> fh_put(ref_fh);
> >>>
> >>> set_version_and_fsid_type will use fh_dentry in ref_fh,
> >>> So it *must* be used with a valid ref_fh.
> >>>
> >>> With this patch, fh_put(ref_fh) will put fh_dentry and set to NULL!!!
> >>>
> >>> Also, pynfs4.1's LKPP1d failed as,
> >>> LKPP1d st_lookupp.testLookupp : FAILURE
> >>> LOOKUPP returned
> >>> '\x01\x00\x07\x00\x02\x00\x00\x00\x00\x00\x00\x004\x93\x1f\x04L*C\x9b\x95L]\x83\x0f\xa4\xbe\x8c',
> >>> expected '\x01\x00\x01\x00\x00\x00\x00\x00'
> >>>
> >>> Move set_version_and_fsid_type back, before fh_put(ref_fh).
> >>>
> >>> thanks,
> >>> Kinglong Mee
> >>>
> >>
> >> Ahh ok, it took me a minute but I see what you're saying now. Yes, this
> >> is broken. I'll fix it, but I'm yet convinced that that is the best way.
> >>
> >> Thanks,
> >> Jeff
> >>
> >
> > Ok, I think this may be a better fix. I'll roll this into the original
> > patch before I send the next respin, but wanted to send it along first
> > to see whether you see any issue with it
> >
> > The idea here is to instead encode the filehandle first, and only do
> > the fh_put for the ref_fh and take dentry/export references if that
> > succeeds.
>
> Must make sure the order as following,
>
> set_version_and_fsid_type(fhp, exp, ref_fh);
> if (ref_fh == fhp)
> fh_put(ref_fh);
> fhp->fh_dentry = dget(dentry); /* our internal copy */
> fhp->fh_export = exp_get(exp);
>
Ok, I think this change should satisfy that requirement.
> >
> > Does that look like a reasonable fix to you, or am I missing something?
> > I'll also make sure I run pynfs against this before I submit the next
> > iteration of the series.
>
> I will look forward to it.
>
I ran pynfs against my original patches and saw the breakage you saw.
This patch seems to have fixed it. I'll roll that change into v3 of the
series.
Thanks for the review so far!
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
