On Thu, 2011-04-14 at 15:10 -0400, Chuck Lever wrote:
> On Apr 14, 2011, at 3:04 PM, Weston Andros Adamson wrote:
>
> > The loop was being restarted after each delegation that has NFS_DELEGATION_NEED_RECLAIM
> > set. Instead, build a temporary list of delegations that need to be freed and free
> > them after the for-each-delegation loop.
> >
> > Signed-off-by: Weston Andros Adamson <dros@xxxxxxxxxx>
> > ---
> > fs/nfs/delegation.c | 16 +++++++++++-----
> > 1 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> > index bbbc6bf..c87fad8 100644
> > --- a/fs/nfs/delegation.c
> > +++ b/fs/nfs/delegation.c
> > @@ -645,8 +645,8 @@ void nfs_delegation_reap_unclaimed(struct nfs_client *clp)
> > struct nfs_delegation *delegation;
> > struct nfs_server *server;
> > struct inode *inode;
> > + LIST_HEAD(tmplist);
> >
> > -restart:
> > rcu_read_lock();
> > list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link) {
> > list_for_each_entry_rcu(delegation, &server->delegations,
> > @@ -659,15 +659,21 @@ restart:
> > continue;
> > delegation = nfs_detach_delegation(NFS_I(inode),
> > server);
> > - rcu_read_unlock();
> > -
> > + /* super_list is unused now that deleg is detached */
> > if (delegation != NULL)
> > - nfs_free_delegation(delegation);
> > + list_add(&delegation->super_list, &tmplist);
> > +
> > iput(inode);
>
> /me idly wonders if that iput() is safe to do inside rcu_read_lock() / rcu_read_unlock().
It isn't....
> > - goto restart;
> > }
> > }
> > rcu_read_unlock();
> > +
> > + while (!list_empty(&tmplist)) {
> > + delegation = list_first_entry(&tmplist, struct nfs_delegation,
> > + super_list);
> > + list_del(&delegation->super_list);
> > + nfs_free_delegation(delegation);
> > + }
> > }
There is also the issue that the inode may have disappeared when we get
to this stage; there is nothing pinning it in memory any more.
One suggestion would be to move the iput into the above loop.
The question is, though, whether or not we get much of a performance
improvement.
One advantage of the current scheme is that we're immediately freeing
the delegation after detaching it. If you queue up the delegations for
freeing, then a process that may need to free a delegation that is at
the end of that list in order to make progress (because it wants to
rename a file, or open it for writing) may have a longer wait during
which the server will be replying NFS4ERR_DELAY.
Do we have any evidence for or against?
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
--
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
