I've been busy patching in things to observe what's going on with my
delegation issues.
I implemented a seq_file interface on the server to walk the
file_hashtbl, and some interesting things have popped up:
This is a nfs4_file that has two open stateids allocated to the same client:
file fh: 82369f70 R:2 F:4 FP:ffff881f24286b58 I:ffff881f6f6a91d0 D:ffff881febf08b40 /home/<stuff>
Deleg: Linux NFSv4.1 <hosta> : 2292335602000000d89e0700 120317
Deleg: Linux NFSv4.1 <hostb> : 2292335607000000437f0700 148262
Stateid: Linux NFSv4.1 <hosta> : 2292335602000000d69e0700/1
Stateid: Linux NFSv4.1 <hosta> : 2292335602000000d79e0700/1
This one has four:
file fh: 6ecdd5f8 R:4 F:4 FP:ffff88083e2974a0 I:ffff881f6c9349d0 D:ffff883f4ad1e780 /home/<stuff>
Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f1620800/1
Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f0620800/1
Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c64c0800/1
Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c74c0800/1
These are nfs4_files that have elevated fi_ref counts but are
essentially "empty":
(note: fi_had_conflict is seemingly always nonzero on entries like these)
file fh: ee23414c F:1 FP:ffff88087fc52318 I:0000000000000000
file fh: 5a8e8c86 F:2 FP:ffff881f5e872318 I:0000000000000000
======================
for clarity:
+static int nfs4_file_hash_list_show(struct seq_file *seq, void *v)
+{
+ char *name = __getname();
+ char *p;
+ int i;
+ struct dentry *dentry;
+ struct inode *inode = NULL;
+ struct nfs4_delegation *dp;
+ struct nfs4_ol_stateid *stp;
+ if (!name)
+ goto out;
+
+ if (v != SEQ_START_TOKEN) {
+ struct nfs4_file *fp = (struct nfs4_file *) v;
+ spin_lock(&fp->fi_lock);
+ seq_printf(seq, "file fh: %08x",nfsd_fhandle_hash(&fp->fi_fhandle));
+ for (i = 0; i < 3; i++) {
+ if (fp->fi_fds[i]) {
+ if (!inode)
+ inode = (fp->fi_fds[i])->f_inode;
+ dentry = (fp->fi_fds[i])->f_path.dentry;
+ if (dentry)
+ break;
+ }
+ }
+ if (atomic_read(&fp->fi_access[O_RDONLY]) > 0)
+ seq_printf(seq, " R:%d",
+ atomic_read(&fp->fi_access[O_RDONLY]));
+ if (atomic_read(&fp->fi_access[O_WRONLY]) > 0)
+ seq_printf(seq, " W:%d",
+ atomic_read(&fp->fi_access[O_WRONLY]));
+ seq_printf(seq, " F:%d",
+ atomic_read(&fp->fi_ref));
+ seq_printf(seq, " FP:%pK", fp);
+ if ((!dentry) && fp->fi_deleg_file)
+ dentry = fp->fi_deleg_file->f_path.dentry;
+ if ((!inode) && fp->fi_deleg_file)
+ inode = (fp->fi_deleg_file)->f_inode;
+ seq_printf(seq, " I:%pK", inode);
+ if (dentry) {
+ p = dentry_path_raw(dentry, name, PATH_MAX);
+ seq_printf(seq, " D:%pK %s\n", dentry, p);
+ } else {
+ seq_printf(seq, "\n");
+ }
+ list_for_each_entry(dp, &fp->fi_delegations, dl_perfile) {
+ seq_printf(seq, " Deleg: %.*s : %*phN %lu\n",
+ (int)dp->dl_recall.cb_clp->cl_name.len,
+ dp->dl_recall.cb_clp->cl_name.data,
+ 12,
+ &dp->dl_stid.sc_stateid.si_opaque,
+ get_seconds() - dp->dl_alloctime);
+ }
+ list_for_each_entry(stp, &fp->fi_stateids, st_perfile) {
+ seq_printf(seq, " Stateid: %.*s : %*phN/%x\n",
+ (int)stp->st_stid.sc_client->cl_name.len,
+ stp->st_stid.sc_client->cl_name.data,
+ 12,
+ &stp->st_stid.sc_stateid.si_opaque,
+ stp->st_stid.sc_stateid.si_generation);
+ }
+ spin_unlock(&fp->fi_lock);
+ }
+ __putname(name);
+out:
+ return 0;
+}
Thanks,
Andy
--
Andrew W. Elble
[email protected]
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
> This one has four:
>
> file fh: 6ecdd5f8 R:4 F:4 FP:ffff88083e2974a0 I:ffff881f6c9349d0
> D:ffff883f4ad1e780 /home/<stuff>
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f1620800/1
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f0620800/1
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c64c0800/1
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c74c0800/1
For clarity, these still are around even after killing any processes that
would have this file opened - and unmounting the filesystem from the client.
Thanks,
Andy
--
Andrew W. Elble
[email protected]
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
On Mon, Nov 02, 2015 at 11:44:27AM -0500, Andrew W Elble wrote:
>
> I've been busy patching in things to observe what's going on with my
> delegation issues.
>
> I implemented a seq_file interface on the server to walk the
> file_hashtbl, and some interesting things have popped up:
Zowie.
So you're waiting for the problem to reproduce itself, and then dumping
this information?
(And if I remember correctly the problem was that the server was
attempting to recall delegations that the client claimed not to have any
knowledge of?)
> This is a nfs4_file that has two open stateids allocated to the same client:
>
> file fh: 82369f70 R:2 F:4 FP:ffff881f24286b58 I:ffff881f6f6a91d0 D:ffff881febf08b40 /home/<stuff>
> Deleg: Linux NFSv4.1 <hosta> : 2292335602000000d89e0700 120317
> Deleg: Linux NFSv4.1 <hostb> : 2292335607000000437f0700 148262
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000d69e0700/1
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000d79e0700/1
>
> This one has four:
>
> file fh: 6ecdd5f8 R:4 F:4 FP:ffff88083e2974a0 I:ffff881f6c9349d0 D:ffff883f4ad1e780 /home/<stuff>
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f1620800/1
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f0620800/1
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c64c0800/1
> Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c74c0800/1
I believe stateid's are per client,openowner,file. So multiple
stateid's per file should be OK if there are multiple openowners.
> These are nfs4_files that have elevated fi_ref counts but are
> essentially "empty":
> (note: fi_had_conflict is seemingly always nonzero on entries like these)
>
> file fh: ee23414c F:1 FP:ffff88087fc52318 I:0000000000000000
> file fh: 5a8e8c86 F:2 FP:ffff881f5e872318 I:0000000000000000
So if I understand correctly, there's neither a delegated file nor
anything in the fi_fds[] array? Hm.
I'll have to stare at the code to figure out what that means.
fi_had_conflict means a delegation was recalled on this file at some
point.
--b.
>
> ======================
>
> for clarity:
>
> +static int nfs4_file_hash_list_show(struct seq_file *seq, void *v)
> +{
> + char *name = __getname();
> + char *p;
> + int i;
> + struct dentry *dentry;
> + struct inode *inode = NULL;
> + struct nfs4_delegation *dp;
> + struct nfs4_ol_stateid *stp;
> + if (!name)
> + goto out;
> +
> + if (v != SEQ_START_TOKEN) {
> + struct nfs4_file *fp = (struct nfs4_file *) v;
> + spin_lock(&fp->fi_lock);
> + seq_printf(seq, "file fh: %08x",nfsd_fhandle_hash(&fp->fi_fhandle));
> + for (i = 0; i < 3; i++) {
> + if (fp->fi_fds[i]) {
> + if (!inode)
> + inode = (fp->fi_fds[i])->f_inode;
> + dentry = (fp->fi_fds[i])->f_path.dentry;
> + if (dentry)
> + break;
> + }
> + }
> + if (atomic_read(&fp->fi_access[O_RDONLY]) > 0)
> + seq_printf(seq, " R:%d",
> + atomic_read(&fp->fi_access[O_RDONLY]));
> + if (atomic_read(&fp->fi_access[O_WRONLY]) > 0)
> + seq_printf(seq, " W:%d",
> + atomic_read(&fp->fi_access[O_WRONLY]));
> + seq_printf(seq, " F:%d",
> + atomic_read(&fp->fi_ref));
> + seq_printf(seq, " FP:%pK", fp);
> + if ((!dentry) && fp->fi_deleg_file)
> + dentry = fp->fi_deleg_file->f_path.dentry;
> + if ((!inode) && fp->fi_deleg_file)
> + inode = (fp->fi_deleg_file)->f_inode;
> + seq_printf(seq, " I:%pK", inode);
> + if (dentry) {
> + p = dentry_path_raw(dentry, name, PATH_MAX);
> + seq_printf(seq, " D:%pK %s\n", dentry, p);
> + } else {
> + seq_printf(seq, "\n");
> + }
> + list_for_each_entry(dp, &fp->fi_delegations, dl_perfile) {
> + seq_printf(seq, " Deleg: %.*s : %*phN %lu\n",
> + (int)dp->dl_recall.cb_clp->cl_name.len,
> + dp->dl_recall.cb_clp->cl_name.data,
> + 12,
> + &dp->dl_stid.sc_stateid.si_opaque,
> + get_seconds() - dp->dl_alloctime);
> + }
> + list_for_each_entry(stp, &fp->fi_stateids, st_perfile) {
> + seq_printf(seq, " Stateid: %.*s : %*phN/%x\n",
> + (int)stp->st_stid.sc_client->cl_name.len,
> + stp->st_stid.sc_client->cl_name.data,
> + 12,
> + &stp->st_stid.sc_stateid.si_opaque,
> + stp->st_stid.sc_stateid.si_generation);
> + }
> + spin_unlock(&fp->fi_lock);
> + }
> + __putname(name);
> +out:
> + return 0;
> +}
>
> Thanks,
>
> Andy
>
> --
> Andrew W. Elble
> [email protected]
> Infrastructure Engineer, Communications Technical Lead
> Rochester Institute of Technology
> PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
> So you're waiting for the problem to reproduce itself, and then dumping
> this information?
Haven't yet caught it. This information is post-problem by a few hours.
(getting closer to catching it with trace events, needed to deal
with tracing buffer filling up)
> (And if I remember correctly the problem was that the server was
> attempting to recall delegations that the client claimed not to have any
> knowledge of?)
Yes.
> I believe stateid's are per client,openowner,file. So multiple
> stateid's per file should be OK if there are multiple openowners.
I'm pretty sure that's the same openowner. (I'm now printing openowner)
> I'll have to stare at the code to figure out what that means.
No guarantees I'm not responsible for the empty nfs4_file cases, so
I wouldn't spend too much time looking for a path to that - But
I'm pretty sure we need something like the below. I'll repost
tomorrow as an RFC once I get some feedback from running it overnight.
---
fs/nfsd/nfs4state.c | 74 +++++++++++++++++++++++++++++++++++------------------
1 file changed, 49 insertions(+), 25 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 340ff365df4d..b3289434750b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3369,6 +3369,27 @@ static const struct nfs4_stateowner_operations openowner_ops = {
.so_free = nfs4_free_openowner,
};
+static struct nfs4_ol_stateid *
+nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
+{
+ struct nfs4_ol_stateid *local, *ret = NULL;
+ struct nfs4_openowner *oo = open->op_openowner;
+
+ lockdep_assert_held(&fp->fi_lock);
+
+ list_for_each_entry(local, &fp->fi_stateids, st_perfile) {
+ /* ignore lock owners */
+ if (local->st_stateowner->so_is_open_owner == 0)
+ continue;
+ if (local->st_stateowner == &oo->oo_owner) {
+ ret = local;
+ atomic_inc(&ret->st_stid.sc_count);
+ break;
+ }
+ }
+ return ret;
+}
+
static struct nfs4_openowner *
alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
struct nfsd4_compound_state *cstate)
@@ -3400,9 +3421,20 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
return ret;
}
-static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, struct nfsd4_open *open) {
+static struct nfs4_ol_stateid *
+init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
+ struct nfsd4_open *open)
+{
+
struct nfs4_openowner *oo = open->op_openowner;
+ struct nfs4_ol_stateid *retstp = NULL;
+ spin_lock(&oo->oo_owner.so_client->cl_lock);
+ spin_lock(&fp->fi_lock);
+
+ retstp = nfsd4_find_existing_open(fp, open);
+ if (retstp)
+ goto out_unlock;
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_OPEN_STID;
INIT_LIST_HEAD(&stp->st_locks);
@@ -3412,12 +3444,13 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
stp->st_access_bmap = 0;
stp->st_deny_bmap = 0;
stp->st_openstp = NULL;
- spin_lock(&oo->oo_owner.so_client->cl_lock);
list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
- spin_lock(&fp->fi_lock);
list_add(&stp->st_perfile, &fp->fi_stateids);
+
+out_unlock:
spin_unlock(&fp->fi_lock);
spin_unlock(&oo->oo_owner.so_client->cl_lock);
+ return retstp;
}
/*
@@ -3828,27 +3861,6 @@ out:
return nfs_ok;
}
-static struct nfs4_ol_stateid *
-nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
-{
- struct nfs4_ol_stateid *local, *ret = NULL;
- struct nfs4_openowner *oo = open->op_openowner;
-
- spin_lock(&fp->fi_lock);
- list_for_each_entry(local, &fp->fi_stateids, st_perfile) {
- /* ignore lock owners */
- if (local->st_stateowner->so_is_open_owner == 0)
- continue;
- if (local->st_stateowner == &oo->oo_owner) {
- ret = local;
- atomic_inc(&ret->st_stid.sc_count);
- break;
- }
- }
- spin_unlock(&fp->fi_lock);
- return ret;
-}
-
static inline int nfs4_access_to_access(u32 nfs4_access)
{
int flags = 0;
@@ -4234,6 +4246,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
struct nfs4_file *fp = NULL;
struct nfs4_ol_stateid *stp = NULL;
+ struct nfs4_ol_stateid *swapstp = NULL;
struct nfs4_delegation *dp = NULL;
__be32 status;
@@ -4247,7 +4260,9 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
status = nfs4_check_deleg(cl, open, &dp);
if (status)
goto out;
+ spin_lock(&fp->fi_lock);
stp = nfsd4_find_existing_open(fp, open);
+ spin_unlock(&fp->fi_lock);
} else {
open->op_file = NULL;
status = nfserr_bad_stateid;
@@ -4267,7 +4282,15 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
} else {
stp = open->op_stp;
open->op_stp = NULL;
- init_open_stateid(stp, fp, open);
+ swapstp = init_open_stateid(stp, fp, open);
+ if (swapstp) {
+ nfs4_put_stid(&stp->st_stid);
+ stp = swapstp;
+ status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
+ if (status)
+ goto out;
+ goto upgrade_out;
+ }
status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
if (status) {
release_open_stateid(stp);
@@ -4279,6 +4302,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
if (stp->st_clnt_odstate == open->op_odstate)
open->op_odstate = NULL;
}
+upgrade_out:
update_stateid(&stp->st_stid.sc_stateid);
memcpy(&open->op_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
--
2.4.6
Thanks,
Andy
--
Andrew W. Elble
[email protected]
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
On Tue, 3 Nov 2015 17:06:24 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Mon, Nov 02, 2015 at 11:44:27AM -0500, Andrew W Elble wrote:
> >
> > I've been busy patching in things to observe what's going on with my
> > delegation issues.
> >
> > I implemented a seq_file interface on the server to walk the
> > file_hashtbl, and some interesting things have popped up:
>
> Zowie.
>
> So you're waiting for the problem to reproduce itself, and then dumping
> this information?
>
> (And if I remember correctly the problem was that the server was
> attempting to recall delegations that the client claimed not to have any
> knowledge of?)
>
> > This is a nfs4_file that has two open stateids allocated to the same client:
> >
> > file fh: 82369f70 R:2 F:4 FP:ffff881f24286b58 I:ffff881f6f6a91d0 D:ffff881febf08b40 /home/<stuff>
> > Deleg: Linux NFSv4.1 <hosta> : 2292335602000000d89e0700 120317
> > Deleg: Linux NFSv4.1 <hostb> : 2292335607000000437f0700 148262
> > Stateid: Linux NFSv4.1 <hosta> : 2292335602000000d69e0700/1
> > Stateid: Linux NFSv4.1 <hosta> : 2292335602000000d79e0700/1
> >
> > This one has four:
> >
> > file fh: 6ecdd5f8 R:4 F:4 FP:ffff88083e2974a0 I:ffff881f6c9349d0 D:ffff883f4ad1e780 /home/<stuff>
> > Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f1620800/1
> > Stateid: Linux NFSv4.1 <hosta> : 2292335602000000f0620800/1
> > Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c64c0800/1
> > Stateid: Linux NFSv4.1 <hosta> : 2292335602000000c74c0800/1
>
> I believe stateid's are per client,openowner,file. So multiple
> stateid's per file should be OK if there are multiple openowners.
>
Technically, there is a per-client hash of openowners and then each
open stateid is per-openowner. But your statement is basically correct.
> > These are nfs4_files that have elevated fi_ref counts but are
> > essentially "empty":
> > (note: fi_had_conflict is seemingly always nonzero on entries like
> > these)
> >
> > file fh: ee23414c F:1 FP:ffff88087fc52318 I:0000000000000000
> > file fh: 5a8e8c86 F:2 FP:ffff881f5e872318 I:0000000000000000
>
> So if I understand correctly, there's neither a delegated file nor
> anything in the fi_fds[] array? Hm.
>
Yes, looks that way. The code sometimes will take "ephemeral"
references to a file after looking it up, for instance. That said, if
they're sticking around for a long time afterward then we may have a
fi_ref leak somewhere.
> I'll have to stare at the code to figure out what that means.
>
> fi_had_conflict means a delegation was recalled on this file at some
> point.
>
Right.
>
> >
> > ======================
> >
> > for clarity:
> >
> > +static int nfs4_file_hash_list_show(struct seq_file *seq, void *v)
> > +{
> > + char *name = __getname();
> > + char *p;
> > + int i;
> > + struct dentry *dentry;
> > + struct inode *inode = NULL;
> > + struct nfs4_delegation *dp;
> > + struct nfs4_ol_stateid *stp;
> > + if (!name)
> > + goto out;
> > +
> > + if (v != SEQ_START_TOKEN) {
> > + struct nfs4_file *fp = (struct nfs4_file *) v;
> > + spin_lock(&fp->fi_lock);
> > + seq_printf(seq, "file fh:
> > %08x",nfsd_fhandle_hash(&fp->fi_fhandle));
> > + for (i = 0; i < 3; i++) {
> > + if (fp->fi_fds[i]) {
> > + if (!inode)
> > + inode =
> > (fp->fi_fds[i])->f_inode;
> > + dentry =
> > (fp->fi_fds[i])->f_path.dentry;
> > + if (dentry)
> > + break;
> > + }
> > + }
> > + if (atomic_read(&fp->fi_access[O_RDONLY]) > 0)
> > + seq_printf(seq, " R:%d",
> > +
> > atomic_read(&fp->fi_access[O_RDONLY]));
> > + if (atomic_read(&fp->fi_access[O_WRONLY]) > 0)
> > + seq_printf(seq, " W:%d",
> > +
> > atomic_read(&fp->fi_access[O_WRONLY]));
> > + seq_printf(seq, " F:%d",
> > + atomic_read(&fp->fi_ref));
> > + seq_printf(seq, " FP:%pK", fp);
> > + if ((!dentry) && fp->fi_deleg_file)
> > + dentry = fp->fi_deleg_file->f_path.dentry;
> > + if ((!inode) && fp->fi_deleg_file)
> > + inode = (fp->fi_deleg_file)->f_inode;
> > + seq_printf(seq, " I:%pK", inode);
> > + if (dentry) {
> > + p = dentry_path_raw(dentry, name, PATH_MAX);
> > + seq_printf(seq, " D:%pK %s\n", dentry, p);
> > + } else {
> > + seq_printf(seq, "\n");
> > + }
> > + list_for_each_entry(dp, &fp->fi_delegations,
> > dl_perfile) {
> > + seq_printf(seq, " Deleg: %.*s : %*phN
> > %lu\n",
> > +
> > (int)dp->dl_recall.cb_clp->cl_name.len,
> > + dp->dl_recall.cb_clp->cl_name.data,
> > + 12,
> > + &dp->dl_stid.sc_stateid.si_opaque,
> > + get_seconds() - dp->dl_alloctime);
> > + }
> > + list_for_each_entry(stp, &fp->fi_stateids,
> > st_perfile) {
> > + seq_printf(seq, " Stateid: %.*s :
> > %*phN/%x\n",
> > +
> > (int)stp->st_stid.sc_client->cl_name.len,
> > +
> > stp->st_stid.sc_client->cl_name.data,
> > + 12,
> > + &stp->st_stid.sc_stateid.si_opaque,
> > +
> > stp->st_stid.sc_stateid.si_generation);
> > + }
> > + spin_unlock(&fp->fi_lock);
> > + }
> > + __putname(name);
> > +out:
> > + return 0;
> > +}
> >
> > Thanks,
> >
> > Andy
> >
> > --
> > Andrew W. Elble
> > [email protected]
> > Infrastructure Engineer, Communications Technical Lead
> > Rochester Institute of Technology
> > PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
--
Jeff Layton <[email protected]>