Oh! The original patch should be dropped. I should have sent a self-NAK
on it before. I'll do that now...
Thanks,
Jeff
On Thu, 2016-04-28 at 16:37 -0400, Anna Schumaker wrote:
> Hi Jeff,
>
> It looks like this patch removes the mirror->uid and mirror->gid
> parameters that your earlier patch (nfs: don't match flexfiles
> mirrors that have different creds) makes use of. Should that patch
> be updated, or does this series make that patch obsolete?
>
> Thanks,
> Anna
>
> On 04/21/2016 08:51 PM, Jeff Layton wrote:
> > We're just as likely to have allocation problems here as we would
> > if we
> > delay looking up the credential like we currently do. Fix the code
> > to
> > get a rpc_cred reference early, as soon as the mirror is set up.
> >
> > This allows us to eliminate the mirror early if there is a problem
> > getting an rpc credential. This also allows us to drop the uid/gid
> > from the layout_mirror struct as well.
> >
> > In the event that we find an existing mirror where this one would
> > go, we
> > swap in the new creds unconditionally, and drop the reference to
> > the old
> > one.
> >
> > Note that the old ff_layout_update_mirror_cred function wouldn't
> > set
> > this pointer unless the DS version was 3, but we don't know what
> > the DS
> > version is at this point. I'm a little unclear on why it did that
> > as you
> > still need creds to talk to v4 servers as well. I have the code set
> > it regardless of the DS version here.
> >
> > Also note the change to using generic creds instead of calling
> > lookup_cred directly. With that change, we also need to populate
> > the
> > group_info pointer in the acred as some functions expect that to
> > never
> > be NULL. Instead of allocating one every time however, we can
> > allocate
> > one when the module is loaded and share it since the group_info is
> > refcounted.
> >
> > Signed-off-by: Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
> > ---
> > fs/nfs/flexfilelayout/flexfilelayout.c | 41
> > ++++++++++++++++++++++-----
> > fs/nfs/flexfilelayout/flexfilelayout.h | 2 --
> > fs/nfs/flexfilelayout/flexfilelayoutdev.c | 47 ++-----------------
> > ------------
> > 3 files changed, 36 insertions(+), 54 deletions(-)
> >
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c
> > b/fs/nfs/flexfilelayout/flexfilelayout.c
> > index 432d5b0009c7..19806caa8ff2 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > @@ -26,6 +26,8 @@
> >
> > #define FF_LAYOUT_POLL_RETRY_MAX (15*HZ)
> >
> > +static struct group_info *ff_zero_group;
> > +
> > static struct pnfs_layout_hdr *
> > ff_layout_alloc_layout_hdr(struct inode *inode, gfp_t gfp_flags)
> > {
> > @@ -407,8 +409,9 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr
> > *lh,
> > struct nfs4_ff_layout_mirror *mirror;
> > struct nfs4_deviceid devid;
> > struct nfs4_deviceid_node *idnode;
> > - u32 ds_count;
> > - u32 fh_count;
> > + struct auth_cred acred = { .group_info =
> > ff_zero_group };
> > + struct rpc_cred *cred;
> > + u32 ds_count, fh_count, id;
> > int j;
> >
> > rc = -EIO;
> > @@ -484,24 +487,39 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr
> > *lh,
> > fls->mirror_array[i]->fh_versions_cnt = fh_count;
> >
> > /* user */
> > - rc = decode_name(&stream, &fls->mirror_array[i]-
> > >uid);
> > + rc = decode_name(&stream, &id);
> > if (rc)
> > goto out_err_free;
> >
> > + acred.uid = make_kuid(&init_user_ns, id);
> > +
> > /* group */
> > - rc = decode_name(&stream, &fls->mirror_array[i]-
> > >gid);
> > + rc = decode_name(&stream, &id);
> > if (rc)
> > goto out_err_free;
> >
> > + acred.gid = make_kgid(&init_user_ns, id);
> > +
> > + /* find the cred for it */
> > + cred = rpc_lookup_generic_cred(&acred, 0,
> > gfp_flags);
> > + if (IS_ERR(cred)) {
> > + rc = PTR_ERR(cred);
> > + goto out_err_free;
> > + }
> > +
> > + rcu_assign_pointer(fls->mirror_array[i]->cred,
> > cred);
> > +
> > mirror = ff_layout_add_mirror(lh, fls-
> > >mirror_array[i]);
> > if (mirror != fls->mirror_array[i]) {
> > + /* swap cred ptrs so free_mirror will
> > clean up old */
> > + fls->mirror_array[i]->cred = xchg(&mirror-
> > >cred, cred);
> > ff_layout_free_mirror(fls-
> > >mirror_array[i]);
> > fls->mirror_array[i] = mirror;
> > }
> >
> > - dprintk("%s: uid %d gid %d\n", __func__,
> > - fls->mirror_array[i]->uid,
> > - fls->mirror_array[i]->gid);
> > + dprintk("%s: uid %u gid %u\n", __func__,
> > + from_kuid(&init_user_ns, acred.uid),
> > + from_kgid(&init_user_ns, acred.gid));
> > }
> >
> > p = xdr_inline_decode(&stream, 4);
> > @@ -2226,6 +2244,11 @@ static int __init
> > nfs4flexfilelayout_init(void)
> > {
> > printk(KERN_INFO "%s: NFSv4 Flexfile Layout Driver
> > Registering...\n",
> > __func__);
> > + if (!ff_zero_group) {
> > + ff_zero_group = groups_alloc(0);
> > + if (!ff_zero_group)
> > + return -ENOMEM;
> > + }
> > return pnfs_register_layoutdriver(&flexfilelayout_type);
> > }
> >
> > @@ -2234,6 +2257,10 @@ static void __exit
> > nfs4flexfilelayout_exit(void)
> > printk(KERN_INFO "%s: NFSv4 Flexfile Layout Driver
> > Unregistering...\n",
> > __func__);
> > pnfs_unregister_layoutdriver(&flexfilelayout_type);
> > + if (ff_zero_group) {
> > + put_group_info(ff_zero_group);
> > + ff_zero_group = NULL;
> > + }
> > }
> >
> > MODULE_ALIAS("nfs-layouttype4-4");
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h
> > b/fs/nfs/flexfilelayout/flexfilelayout.h
> > index dd353bb7dc0a..c29fc853ce74 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.h
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.h
> > @@ -76,8 +76,6 @@ struct nfs4_ff_layout_mirror {
> > u32 fh_versions_cnt;
> > struct nfs_fh *fh_versions;
> > nfs4_stateid stateid;
> > - u32 uid;
> > - u32 gid;
> > struct rpc_cred *cred;
> > atomic_t ref;
> > spinlock_t lock;
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
> > b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
> > index baee22929174..6ddd8a5c5ae0 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
> > @@ -302,42 +302,6 @@ int ff_layout_track_ds_error(struct
> > nfs4_flexfile_layout *flo,
> > return 0;
> > }
> >
> > -/* currently we only support AUTH_NONE and AUTH_SYS */
> > -static rpc_authflavor_t
> > -nfs4_ff_layout_choose_authflavor(struct nfs4_ff_layout_mirror
> > *mirror)
> > -{
> > - if (mirror->uid == (u32)-1)
> > - return RPC_AUTH_NULL;
> > - return RPC_AUTH_UNIX;
> > -}
> > -
> > -/* fetch cred for NFSv3 DS */
> > -static int ff_layout_update_mirror_cred(struct
> > nfs4_ff_layout_mirror *mirror,
> > - struct nfs4_pnfs_ds *ds)
> > -{
> > - if (ds->ds_clp && !mirror->cred &&
> > - mirror->mirror_ds->ds_versions[0].version == 3) {
> > - struct rpc_auth *auth = ds->ds_clp->cl_rpcclient-
> > >cl_auth;
> > - struct rpc_cred *cred;
> > - struct auth_cred acred = {
> > - .uid = make_kuid(&init_user_ns, mirror-
> > >uid),
> > - .gid = make_kgid(&init_user_ns, mirror-
> > >gid),
> > - };
> > -
> > - /* AUTH_NULL ignores acred */
> > - cred = auth->au_ops->lookup_cred(auth, &acred, 0);
> > - if (IS_ERR(cred)) {
> > - dprintk("%s: lookup_cred failed with
> > %ld\n",
> > - __func__, PTR_ERR(cred));
> > - return PTR_ERR(cred);
> > - } else {
> > - if (cmpxchg(&mirror->cred, NULL, cred))
> > - put_rpccred(cred);
> > - }
> > - }
> > - return 0;
> > -}
> > -
> > static struct rpc_cred *
> > ff_layout_get_mirror_cred(struct nfs4_ff_layout_mirror *mirror,
> > u32 iomode)
> > {
> > @@ -386,7 +350,6 @@ nfs4_ff_layout_prepare_ds(struct
> > pnfs_layout_segment *lseg, u32 ds_idx,
> > struct inode *ino = lseg->pls_layout->plh_inode;
> > struct nfs_server *s = NFS_SERVER(ino);
> > unsigned int max_payload;
> > - rpc_authflavor_t flavor;
> >
> > if (!ff_layout_mirror_valid(lseg, mirror)) {
> > pr_err_ratelimited("NFS: %s: No data server for
> > offset index %d\n",
> > @@ -402,9 +365,7 @@ nfs4_ff_layout_prepare_ds(struct
> > pnfs_layout_segment *lseg, u32 ds_idx,
> > /* matching smp_wmb() in _nfs4_pnfs_v3/4_ds_connect */
> > smp_rmb();
> > if (ds->ds_clp)
> > - goto out_update_creds;
> > -
> > - flavor = nfs4_ff_layout_choose_authflavor(mirror);
> > + goto out;
> >
> > /* FIXME: For now we assume the server sent only one
> > version of NFS
> > * to use for the DS.
> > @@ -413,7 +374,7 @@ nfs4_ff_layout_prepare_ds(struct
> > pnfs_layout_segment *lseg, u32 ds_idx,
> > dataserver_retrans,
> > mirror->mirror_ds-
> > >ds_versions[0].version,
> > mirror->mirror_ds-
> > >ds_versions[0].minor_version,
> > - flavor);
> > + RPC_AUTH_UNIX);
> >
> > /* connect success, check rsize/wsize limit */
> > if (ds->ds_clp) {
> > @@ -438,11 +399,7 @@ nfs4_ff_layout_prepare_ds(struct
> > pnfs_layout_segment *lseg, u32 ds_idx,
> > } else
> > pnfs_error_mark_layout_for_return(ino,
> > lseg);
> > ds = NULL;
> > - goto out;
> > }
> > -out_update_creds:
> > - if (ff_layout_update_mirror_cred(mirror, ds))
> > - ds = NULL;
> > out:
> > return ds;
> > }
> >
>
> --
> 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
--
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
