Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-ig0-f171.google.com ([209.85.213.171]:52128 "EHLO mail-ig0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751201AbaHOOsK convert rfc822-to-8bit (ORCPT ); Fri, 15 Aug 2014 10:48:10 -0400 Received: by mail-ig0-f171.google.com with SMTP id l13so2103426iga.16 for ; Fri, 15 Aug 2014 07:48:10 -0700 (PDT) Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\)) Subject: Re: [PATCH 3/5] pnfs: find swapped pages on pnfs commit lists too From: Weston Andros Adamson In-Reply-To: <583CC103-DC3D-495D-8C15-473BB8D2998D@primarydata.com> Date: Fri, 15 Aug 2014 10:48:07 -0400 Cc: Trond Myklebust , linux-nfs list Message-Id: <54F24925-5237-4090-A4E4-EDA43F1DD2B8@primarydata.com> References: <1407510816-7002-1-git-send-email-dros@primarydata.com> <1407510816-7002-4-git-send-email-dros@primarydata.com> <53E8C593.6030705@gmail.com> <583CC103-DC3D-495D-8C15-473BB8D2998D@primarydata.com> To: Anna Schumaker Sender: linux-nfs-owner@vger.kernel.org List-ID: For some reason I can?t reproduce this with: CONFIG_NFS_FS=m CONFIG_NFS_V2=m # CONFIG_NFS_V3 is not set # CONFIG_NFS_V4 is not set Are you compiling with some special option? It?s not in the output of make W=1, W=3 or W=3. It looks like there are several places that call nfs_init_cinfo_from_inode without any ifdef logic? We could just fix all of them and be done with it, but I?m wondering why you get the warning and I don?t. -dros On Aug 11, 2014, at 10:54 PM, Weston Andros Adamson wrote: > Thanks Anna, I?ll fix it up. > > -dros > > > > On Aug 11, 2014, at 9:30 AM, Anna Schumaker wrote: > >> On 08/08/2014 11:13 AM, Weston Andros Adamson wrote: >>> nfs_page_find_head_request_locked looks through the regular nfs commit lists >>> when the page is swapped out, but doesn't look through the pnfs commit lists. >>> >>> I'm not sure if anyone has hit any issues caused by this. >>> >>> Suggested-by: Peng Tao >>> Signed-off-by: Weston Andros Adamson >>> Signed-off-by: Trond Myklebust >>> --- >>> fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++ >>> fs/nfs/pnfs.h | 20 +++++++++++++++++ >>> fs/nfs/write.c | 49 +++++++++++++++++++++++++++++++----------- >>> 3 files changed, 88 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c >>> index 2576d28b..524e66f 100644 >>> --- a/fs/nfs/filelayout/filelayout.c >>> +++ b/fs/nfs/filelayout/filelayout.c >>> @@ -1237,6 +1237,36 @@ restart: >>> spin_unlock(cinfo->lock); >>> } >>> >>> +/* filelayout_search_commit_reqs - Search lists in @cinfo for the head reqest >>> + * for @page >>> + * @cinfo - commit info for current inode >>> + * @page - page to search for matching head request >>> + * >>> + * Returns a the head request if one is found, otherwise returns NULL. >>> + */ >>> +static struct nfs_page * >>> +filelayout_search_commit_reqs(struct nfs_commit_info *cinfo, struct page *page) >>> +{ >>> + struct nfs_page *freq, *t; >>> + struct pnfs_commit_bucket *b; >>> + int i; >>> + >>> + /* Linearly search the commit lists for each bucket until a matching >>> + * request is found */ >>> + for (i = 0, b = cinfo->ds->buckets; i < cinfo->ds->nbuckets; i++, b++) { >>> + list_for_each_entry_safe(freq, t, &b->written, wb_list) { >>> + if (freq->wb_page == page) >>> + return freq->wb_head; >>> + } >>> + list_for_each_entry_safe(freq, t, &b->committing, wb_list) { >>> + if (freq->wb_page == page) >>> + return freq->wb_head; >>> + } >>> + } >>> + >>> + return NULL; >>> +} >>> + >>> static void filelayout_retry_commit(struct nfs_commit_info *cinfo, int idx) >>> { >>> struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds; >>> @@ -1386,6 +1416,7 @@ static struct pnfs_layoutdriver_type filelayout_type = { >>> .clear_request_commit = filelayout_clear_request_commit, >>> .scan_commit_lists = filelayout_scan_commit_lists, >>> .recover_commit_reqs = filelayout_recover_commit_reqs, >>> + .search_commit_reqs = filelayout_search_commit_reqs, >>> .commit_pagelist = filelayout_commit_pagelist, >>> .read_pagelist = filelayout_read_pagelist, >>> .write_pagelist = filelayout_write_pagelist, >>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h >>> index 27ddecd..203b6c9 100644 >>> --- a/fs/nfs/pnfs.h >>> +++ b/fs/nfs/pnfs.h >>> @@ -104,6 +104,8 @@ struct pnfs_layoutdriver_type { >>> int max); >>> void (*recover_commit_reqs) (struct list_head *list, >>> struct nfs_commit_info *cinfo); >>> + struct nfs_page * (*search_commit_reqs)(struct nfs_commit_info *cinfo, >>> + struct page *page); >>> int (*commit_pagelist)(struct inode *inode, >>> struct list_head *mds_pages, >>> int how, >>> @@ -341,6 +343,17 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list, >>> NFS_SERVER(inode)->pnfs_curr_ld->recover_commit_reqs(list, cinfo); >>> } >>> >>> +static inline struct nfs_page * >>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo, >>> + struct page *page) >>> +{ >>> + struct pnfs_layoutdriver_type *ld = NFS_SERVER(inode)->pnfs_curr_ld; >>> + >>> + if (ld == NULL || ld->search_commit_reqs == NULL) >>> + return NULL; >>> + return ld->search_commit_reqs(cinfo, page); >>> +} >>> + >>> /* Should the pNFS client commit and return the layout upon a setattr */ >>> static inline bool >>> pnfs_ld_layoutret_on_setattr(struct inode *inode) >>> @@ -492,6 +505,13 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list, >>> { >>> } >>> >>> +static inline struct nfs_page * >>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo, >>> + struct page *page) >>> +{ >>> + return NULL; >>> +} >>> + >>> static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync) >>> { >>> return 0; >>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c >>> index e6bc5b5..ba41769 100644 >>> --- a/fs/nfs/write.c >>> +++ b/fs/nfs/write.c >>> @@ -47,6 +47,8 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops; >>> static const struct nfs_commit_completion_ops nfs_commit_completion_ops; >>> static const struct nfs_rw_ops nfs_rw_write_ops; >>> static void nfs_clear_request_commit(struct nfs_page *req); >>> +static void nfs_init_cinfo_from_inode(struct nfs_commit_info *cinfo, >>> + struct inode *inode); >>> >>> static struct kmem_cache *nfs_wdata_cachep; >>> static mempool_t *nfs_wdata_mempool; >>> @@ -93,6 +95,38 @@ static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error) >>> } >>> >>> /* >>> + * nfs_page_search_commits_for_head_request_locked >>> + * >>> + * Search through commit lists on @inode for the head request for @page. >>> + * Must be called while holding the inode (which is cinfo) lock. >>> + * >>> + * Returns the head request if found, or NULL if not found. >>> + */ >>> +static struct nfs_page * >>> +nfs_page_search_commits_for_head_request_locked(struct nfs_inode *nfsi, >>> + struct page *page) >>> +{ >>> + struct nfs_page *freq, *t; >>> + struct nfs_commit_info cinfo; >>> + struct inode *inode = &nfsi->vfs_inode; >>> + >>> + nfs_init_cinfo_from_inode(&cinfo, inode); >> This has a compiler warning when CONFIG_NFS_V4=n, CONFIG_NFS_V3=n, CONFIG_NFS_V2=y: >> >> fs/nfs/write.c: In function ?nfs_page_find_head_request_locked.isra.16?: >> include/linux/kernel.h:834:39: error: ?cinfo.mds? may be used uninitialized in this function [-Werror=maybe-uninitialized] >> const typeof( ((type *)0)->member ) *__mptr = (ptr); \ >> ^ >> fs/nfs/write.c:110:25: note: ?cinfo.mds? was declared here >> struct nfs_commit_info cinfo; >> >> Probably because nfs_init_cinfo_from_inode() is empty without v3 or v4. >> >> Anna >>> + >>> + /* search through pnfs commit lists */ >>> + freq = pnfs_search_commit_reqs(inode, &cinfo, page); >>> + if (freq) >>> + return freq->wb_head; >>> + >>> + /* Linearly search the commit list for the correct request */ >>> + list_for_each_entry_safe(freq, t, &cinfo.mds->list, wb_list) { >>> + if (freq->wb_page == page) >>> + return freq->wb_head; >>> + } >>> + >>> + return NULL; >>> +} >>> + >>> +/* >>> * nfs_page_find_head_request_locked - find head request associated with @page >>> * >>> * must be called while holding the inode lock. >>> @@ -106,21 +140,12 @@ nfs_page_find_head_request_locked(struct nfs_inode *nfsi, struct page *page) >>> >>> if (PagePrivate(page)) >>> req = (struct nfs_page *)page_private(page); >>> - else if (unlikely(PageSwapCache(page))) { >>> - struct nfs_page *freq, *t; >>> - >>> - /* Linearly search the commit list for the correct req */ >>> - list_for_each_entry_safe(freq, t, &nfsi->commit_info.list, wb_list) { >>> - if (freq->wb_page == page) { >>> - req = freq->wb_head; >>> - break; >>> - } >>> - } >>> - } >>> + else if (unlikely(PageSwapCache(page))) >>> + req = nfs_page_search_commits_for_head_request_locked(nfsi, >>> + page); >>> >>> if (req) { >>> WARN_ON_ONCE(req->wb_head != req); >>> - >>> kref_get(&req->wb_kref); >>> } >