Return-Path: Received: from mx2.netapp.com ([216.240.18.37]:60171 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758911Ab1D0WiR convert rfc822-to-8bit (ORCPT ); Wed, 27 Apr 2011 18:38:17 -0400 Subject: Re: [PATCH] NFS: move pnfs layouts to nfs_server structure Content-Type: text/plain; charset=us-ascii From: Dros Adamson In-Reply-To: <4DB87D31.2060808@panasas.com> Date: Wed, 27 Apr 2011 18:38:06 -0400 Cc: trond@netapp.com, linux-nfs@vger.kernel.org Message-Id: References: <1303922649-25478-1-git-send-email-dros@netapp.com> <4DB8659A.8020206@panasas.com> <4DB87D31.2060808@panasas.com> To: Benny Halevy Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Apr 27, 2011, at 4:31 PM, Benny Halevy wrote: > On 2011-04-27 21:56, Dros Adamson wrote: >> >> On Apr 27, 2011, at 2:51 PM, Benny Halevy wrote: >> >>> On 2011-04-27 19:44, Weston Andros Adamson wrote: >>>> Layouts should be tracked per FSID (aka superblock, aka struct nfs_server) >>>> instead of per struct nfs_client, which may have multiple FSIDs associated >>>> with it. >>>> >>>> Signed-off-by: Weston Andros Adamson >>>> --- >>>> fs/nfs/callback_proc.c | 60 +++++++++++++++++++++++++++----------------- >>>> fs/nfs/client.c | 6 ++-- >>>> fs/nfs/pnfs.c | 13 +++++++-- >>>> include/linux/nfs_fs_sb.h | 2 +- >>>> 4 files changed, 51 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c >>>> index 2f41dcce..3964467 100644 >>>> --- a/fs/nfs/callback_proc.c >>>> +++ b/fs/nfs/callback_proc.c >>>> @@ -111,6 +111,7 @@ int nfs4_validate_delegation_stateid(struct nfs_delegation *delegation, const nf >>>> static u32 initiate_file_draining(struct nfs_client *clp, >>>> struct cb_layoutrecallargs *args) >>>> { >>>> + struct nfs_server *server; >>>> struct pnfs_layout_hdr *lo; >>>> struct inode *ino; >>>> bool found = false; >>>> @@ -118,21 +119,28 @@ static u32 initiate_file_draining(struct nfs_client *clp, >>>> LIST_HEAD(free_me_list); >>>> >>>> spin_lock(&clp->cl_lock); >>>> - list_for_each_entry(lo, &clp->cl_layouts, plh_layouts) { >>>> - if (nfs_compare_fh(&args->cbl_fh, >>>> - &NFS_I(lo->plh_inode)->fh)) >>>> - continue; >>>> - ino = igrab(lo->plh_inode); >>>> - if (!ino) >>>> - continue; >>>> - found = true; >>>> - /* Without this, layout can be freed as soon >>>> - * as we release cl_lock. >>>> - */ >>>> - get_layout_hdr(lo); >>>> - break; >>>> + rcu_read_lock(); >>>> + list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link) { >>>> + list_for_each_entry(lo, &server->layouts, plh_layouts) { >>>> + if (nfs_compare_fh(&args->cbl_fh, >>>> + &NFS_I(lo->plh_inode)->fh)) >>>> + continue; >>>> + ino = igrab(lo->plh_inode); >>>> + if (!ino) >>>> + continue; >>>> + found = true; >>>> + /* Without this, layout can be freed as soon >>>> + * as we release cl_lock. >>>> + */ >>>> + get_layout_hdr(lo); >>>> + break; >>>> + } >>>> + if (found) >>>> + break; >>>> } >>>> + rcu_read_unlock(); >>>> spin_unlock(&clp->cl_lock); >>>> + >>>> if (!found) >>>> return NFS4ERR_NOMATCHING_LAYOUT; >>>> >>>> @@ -154,6 +162,7 @@ static u32 initiate_file_draining(struct nfs_client *clp, >>>> static u32 initiate_bulk_draining(struct nfs_client *clp, >>>> struct cb_layoutrecallargs *args) >>>> { >>>> + struct nfs_server *server; >>>> struct pnfs_layout_hdr *lo; >>>> struct inode *ino; >>>> u32 rv = NFS4ERR_NOMATCHING_LAYOUT; >>>> @@ -167,18 +176,23 @@ static u32 initiate_bulk_draining(struct nfs_client *clp, >>>> }; >>>> >>>> spin_lock(&clp->cl_lock); >>>> - list_for_each_entry(lo, &clp->cl_layouts, plh_layouts) { >>>> - if ((args->cbl_recall_type == RETURN_FSID) && >>>> - memcmp(&NFS_SERVER(lo->plh_inode)->fsid, >>>> - &args->cbl_fsid, sizeof(struct nfs_fsid))) >>>> - continue; >>>> - if (!igrab(lo->plh_inode)) >>>> - continue; >>>> - get_layout_hdr(lo); >>>> - BUG_ON(!list_empty(&lo->plh_bulk_recall)); >>>> - list_add(&lo->plh_bulk_recall, &recall_list); >>>> + rcu_read_lock(); >>>> + list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link) { >>>> + list_for_each_entry(lo, &server->layouts, plh_layouts) { >>>> + if ((args->cbl_recall_type == RETURN_FSID) && >>>> + memcmp(&server->fsid, &args->cbl_fsid, >>>> + sizeof(struct nfs_fsid))) >>>> + continue; >>> >>> This condition can be popped up to the outer loop now... >>> (or break rather than continue) >>> >>> Benny >> >> Good point. Fred wanted me to wait until this patch got in to optimize handling RETURN_FSID. Comments? > > In this case, I think it is overly careful since the mechanical > change of indenting the whole block in the inner loop doesn't > make sense here. The whole purpose of this patch, as mentioned > in the description is to track layouts per fsid, therefore > testing the fsid at the right level need not be separated > into a separate, optimizing patch, but rather it can be included > in this one as a self-sufficient version. > > Benny OK, I buy it! Patch on the way. -dros