From: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> To: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Wu Fengguang <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>, Steve Rago <sar-a+KepyhlMvJWk0Htik3J/w@public.gmane.org>, Jens Axboe <jens.axboe-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>, Peter Staubach <staubach-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Arjan van de Ven <arjan-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>, Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> Subject: Re: [PATCH 09/12] NFS: Replace __nfs_write_mapping with sync_inode() Date: Tue, 26 Jan 2010 18:17:39 -0500 [thread overview] Message-ID: <1264547859.3788.37.camel@localhost> (raw) In-Reply-To: <20100126112148.GA25170-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> On Tue, 2010-01-26 at 06:21 -0500, Christoph Hellwig wrote: > On Mon, Jan 25, 2010 at 05:15:45PM -0500, Trond Myklebust wrote: > > int nfs_wb_nocommit(struct inode *inode) > > { > > - return nfs_write_mapping(inode->i_mapping, FLUSH_NOCOMMIT); > > + return filemap_write_and_wait(inode->i_mapping); > > Any point in keeping this as a wrapper for a single well-documented > caller? Also taking i_mutex around it seems a bit questionable these > days given that filemap_write_and_wait avoids lifelocks with writing > applications okay and we use it without i_mutex all over the place. > I'm replacing the former patch with the following updated version. Cheers Trond --------------------------------------------------------------------------------------------- NFS: Replace __nfs_write_mapping with sync_inode() From: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> Now that we have correct COMMIT semantics in writeback_single_inode, we can reduce and simplify nfs_wb_all(). Also replace nfs_wb_nocommit() with a call to filemap_write_and_wait(), which doesn't need to hold the inode->i_mutex. With that done, we can eliminate nfs_write_mapping() altogether. Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> --- fs/nfs/inode.c | 15 +++++---------- fs/nfs/write.c | 42 +++++------------------------------------- include/linux/nfs_fs.h | 2 -- 3 files changed, 10 insertions(+), 49 deletions(-) diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 8341709..733cb27 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -495,17 +495,11 @@ int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME; int err; - /* - * Flush out writes to the server in order to update c/mtime. - * - * Hold the i_mutex to suspend application writes temporarily; - * this prevents long-running writing applications from blocking - * nfs_wb_nocommit. - */ + /* Flush out writes to the server in order to update c/mtime. */ if (S_ISREG(inode->i_mode)) { - mutex_lock(&inode->i_mutex); - nfs_wb_nocommit(inode); - mutex_unlock(&inode->i_mutex); + err = filemap_write_and_wait(inode->i_mapping); + if (err) + goto out; } /* @@ -529,6 +523,7 @@ int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) generic_fillattr(inode, stat); stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode)); } +out: return err; } diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 53df027..c28cf57 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1443,7 +1443,6 @@ long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_contr pgoff_t idx_start, idx_end; unsigned int npages = 0; LIST_HEAD(head); - int nocommit = how & FLUSH_NOCOMMIT; long pages, ret; /* FIXME */ @@ -1460,14 +1459,11 @@ long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_contr npages = 0; } } - how &= ~FLUSH_NOCOMMIT; spin_lock(&inode->i_lock); do { ret = nfs_wait_on_requests_locked(inode, idx_start, npages); if (ret != 0) continue; - if (nocommit) - break; pages = nfs_scan_commit(inode, &head, idx_start, npages); if (pages == 0) break; @@ -1481,47 +1477,19 @@ long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_contr return ret; } -static int __nfs_write_mapping(struct address_space *mapping, struct writeback_control *wbc, int how) -{ - int ret; - - ret = nfs_writepages(mapping, wbc); - if (ret < 0) - goto out; - ret = nfs_sync_mapping_wait(mapping, wbc, how); - if (ret < 0) - goto out; - return 0; -out: - __mark_inode_dirty(mapping->host, I_DIRTY_PAGES); - return ret; -} - -/* Two pass sync: first using WB_SYNC_NONE, then WB_SYNC_ALL */ -static int nfs_write_mapping(struct address_space *mapping, int how) +/* + * flush the inode to disk. + */ +int nfs_wb_all(struct inode *inode) { struct writeback_control wbc = { - .bdi = mapping->backing_dev_info, .sync_mode = WB_SYNC_ALL, .nr_to_write = LONG_MAX, .range_start = 0, .range_end = LLONG_MAX, }; - return __nfs_write_mapping(mapping, &wbc, how); -} - -/* - * flush the inode to disk. - */ -int nfs_wb_all(struct inode *inode) -{ - return nfs_write_mapping(inode->i_mapping, 0); -} - -int nfs_wb_nocommit(struct inode *inode) -{ - return nfs_write_mapping(inode->i_mapping, FLUSH_NOCOMMIT); + return sync_inode(inode, &wbc); } int nfs_wb_page_cancel(struct inode *inode, struct page *page) diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 1eec414..3383622 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -33,7 +33,6 @@ #define FLUSH_STABLE 4 /* commit to stable storage */ #define FLUSH_LOWPRI 8 /* low priority background flush */ #define FLUSH_HIGHPRI 16 /* high priority memory reclaim flush */ -#define FLUSH_NOCOMMIT 32 /* Don't send the NFSv3/v4 COMMIT */ #ifdef __KERNEL__ @@ -477,7 +476,6 @@ extern int nfs_writeback_done(struct rpc_task *, struct nfs_write_data *); */ extern long nfs_sync_mapping_wait(struct address_space *, struct writeback_control *, int); extern int nfs_wb_all(struct inode *inode); -extern int nfs_wb_nocommit(struct inode *inode); extern int nfs_wb_page(struct inode *inode, struct page* page); extern int nfs_wb_page_cancel(struct inode *inode, struct page* page); #if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4) -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Trond Myklebust <Trond.Myklebust@netapp.com> To: Christoph Hellwig <hch@infradead.org> Cc: linux-nfs@vger.kernel.org, Wu Fengguang <fengguang.wu@intel.com>, Peter Zijlstra <peterz@infradead.org>, Jan Kara <jack@suse.cz>, Steve Rago <sar-a+KepyhlMvJWk0Htik3J/w@public.gmane.org>, Jens Axboe <jens.axboe@oracle.com>, Peter Staubach <staubach@redhat.com>, Arjan van de Ven <arjan@infradead.org>, Ingo Molnar <mingo@elte.hu>, linux-fsdevel@vger.kernel.org, Christoph Hellwig <hch@lst.de>, Al Viro <viro@ZenIV.linux.org.uk> Subject: Re: [PATCH 09/12] NFS: Replace __nfs_write_mapping with sync_inode() Date: Tue, 26 Jan 2010 18:17:39 -0500 [thread overview] Message-ID: <1264547859.3788.37.camel@localhost> (raw) In-Reply-To: <20100126112148.GA25170@infradead.org> On Tue, 2010-01-26 at 06:21 -0500, Christoph Hellwig wrote: > On Mon, Jan 25, 2010 at 05:15:45PM -0500, Trond Myklebust wrote: > > int nfs_wb_nocommit(struct inode *inode) > > { > > - return nfs_write_mapping(inode->i_mapping, FLUSH_NOCOMMIT); > > + return filemap_write_and_wait(inode->i_mapping); > > Any point in keeping this as a wrapper for a single well-documented > caller? Also taking i_mutex around it seems a bit questionable these > days given that filemap_write_and_wait avoids lifelocks with writing > applications okay and we use it without i_mutex all over the place. > I'm replacing the former patch with the following updated version. Cheers Trond --------------------------------------------------------------------------------------------- NFS: Replace __nfs_write_mapping with sync_inode() From: Trond Myklebust <Trond.Myklebust@netapp.com> Now that we have correct COMMIT semantics in writeback_single_inode, we can reduce and simplify nfs_wb_all(). Also replace nfs_wb_nocommit() with a call to filemap_write_and_wait(), which doesn't need to hold the inode->i_mutex. With that done, we can eliminate nfs_write_mapping() altogether. Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> --- fs/nfs/inode.c | 15 +++++---------- fs/nfs/write.c | 42 +++++------------------------------------- include/linux/nfs_fs.h | 2 -- 3 files changed, 10 insertions(+), 49 deletions(-) diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 8341709..733cb27 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -495,17 +495,11 @@ int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME; int err; - /* - * Flush out writes to the server in order to update c/mtime. - * - * Hold the i_mutex to suspend application writes temporarily; - * this prevents long-running writing applications from blocking - * nfs_wb_nocommit. - */ + /* Flush out writes to the server in order to update c/mtime. */ if (S_ISREG(inode->i_mode)) { - mutex_lock(&inode->i_mutex); - nfs_wb_nocommit(inode); - mutex_unlock(&inode->i_mutex); + err = filemap_write_and_wait(inode->i_mapping); + if (err) + goto out; } /* @@ -529,6 +523,7 @@ int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) generic_fillattr(inode, stat); stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode)); } +out: return err; } diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 53df027..c28cf57 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1443,7 +1443,6 @@ long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_contr pgoff_t idx_start, idx_end; unsigned int npages = 0; LIST_HEAD(head); - int nocommit = how & FLUSH_NOCOMMIT; long pages, ret; /* FIXME */ @@ -1460,14 +1459,11 @@ long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_contr npages = 0; } } - how &= ~FLUSH_NOCOMMIT; spin_lock(&inode->i_lock); do { ret = nfs_wait_on_requests_locked(inode, idx_start, npages); if (ret != 0) continue; - if (nocommit) - break; pages = nfs_scan_commit(inode, &head, idx_start, npages); if (pages == 0) break; @@ -1481,47 +1477,19 @@ long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_contr return ret; } -static int __nfs_write_mapping(struct address_space *mapping, struct writeback_control *wbc, int how) -{ - int ret; - - ret = nfs_writepages(mapping, wbc); - if (ret < 0) - goto out; - ret = nfs_sync_mapping_wait(mapping, wbc, how); - if (ret < 0) - goto out; - return 0; -out: - __mark_inode_dirty(mapping->host, I_DIRTY_PAGES); - return ret; -} - -/* Two pass sync: first using WB_SYNC_NONE, then WB_SYNC_ALL */ -static int nfs_write_mapping(struct address_space *mapping, int how) +/* + * flush the inode to disk. + */ +int nfs_wb_all(struct inode *inode) { struct writeback_control wbc = { - .bdi = mapping->backing_dev_info, .sync_mode = WB_SYNC_ALL, .nr_to_write = LONG_MAX, .range_start = 0, .range_end = LLONG_MAX, }; - return __nfs_write_mapping(mapping, &wbc, how); -} - -/* - * flush the inode to disk. - */ -int nfs_wb_all(struct inode *inode) -{ - return nfs_write_mapping(inode->i_mapping, 0); -} - -int nfs_wb_nocommit(struct inode *inode) -{ - return nfs_write_mapping(inode->i_mapping, FLUSH_NOCOMMIT); + return sync_inode(inode, &wbc); } int nfs_wb_page_cancel(struct inode *inode, struct page *page) diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 1eec414..3383622 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -33,7 +33,6 @@ #define FLUSH_STABLE 4 /* commit to stable storage */ #define FLUSH_LOWPRI 8 /* low priority background flush */ #define FLUSH_HIGHPRI 16 /* high priority memory reclaim flush */ -#define FLUSH_NOCOMMIT 32 /* Don't send the NFSv3/v4 COMMIT */ #ifdef __KERNEL__ @@ -477,7 +476,6 @@ extern int nfs_writeback_done(struct rpc_task *, struct nfs_write_data *); */ extern long nfs_sync_mapping_wait(struct address_space *, struct writeback_control *, int); extern int nfs_wb_all(struct inode *inode); -extern int nfs_wb_nocommit(struct inode *inode); extern int nfs_wb_page(struct inode *inode, struct page* page); extern int nfs_wb_page_cancel(struct inode *inode, struct page* page); #if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)
next prev parent reply other threads:[~2010-01-26 23:17 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-01-25 22:15 [PATCH 00/12] Re: [PATCH] improve the performance of large sequential write NFS workloads Trond Myklebust 2010-01-25 22:15 ` [PATCH 05/12] VM/NFS: The VM must tell the filesystem when to free reclaimable pages Trond Myklebust [not found] ` <20100125221544.16750.70574.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2010-01-25 22:15 ` [PATCH 04/12] NFS: Reduce the number of unnecessary COMMIT calls Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 03/12] NFS: Cleanup - move nfs_write_inode() into fs/nfs/write.c Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 06/12] NFS: Run COMMIT as an asynchronous RPC call when wbc->for_background is set Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 09/12] NFS: Replace __nfs_write_mapping with sync_inode() Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust [not found] ` <20100125221545.16750.63968.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2010-01-26 11:21 ` Christoph Hellwig 2010-01-26 11:21 ` Christoph Hellwig [not found] ` <20100126112148.GA25170-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 2010-01-26 14:02 ` Trond Myklebust 2010-01-26 14:02 ` Trond Myklebust 2010-01-26 23:17 ` Trond Myklebust [this message] 2010-01-26 23:17 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 07/12] NFS: Ensure inode is always marked I_DIRTY_DATASYNC, if it has unstable pages Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 01/12] VM: Split out the accounting of unstable writes from BDI_RECLAIMABLE Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 02/12] VM: Don't call bdi_stat(BDI_UNSTABLE) on non-nfs backing-devices Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 08/12] NFS: Simplify nfs_wb_page_cancel() Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 10/12] NFS: Simplify nfs_wb_page() Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust [not found] ` <20100125221545.16750.19154.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2010-03-10 18:51 ` J. R. Okajima 2010-03-10 18:51 ` J. R. Okajima 2010-03-10 19:31 ` Trond Myklebust 2010-03-10 19:31 ` Trond Myklebust [not found] ` <1268249482.3096.76.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2010-03-10 20:18 ` Trond Myklebust 2010-03-10 20:18 ` Trond Myklebust [not found] ` <1268252300.3096.81.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2010-03-11 4:45 ` J. R. Okajima 2010-03-11 4:45 ` J. R. Okajima 2010-03-11 14:26 ` Trond Myklebust 2010-03-11 14:26 ` Trond Myklebust [not found] ` <1268317582.3354.9.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2010-03-12 4:22 ` J. R. Okajima 2010-03-12 4:22 ` J. R. Okajima 2010-03-17 16:49 ` Christoph Hellwig 2010-03-17 16:49 ` Christoph Hellwig 2010-03-17 17:26 ` Trond Myklebust 2010-03-17 17:52 ` Jeff Layton 2010-03-17 17:58 ` Trond Myklebust [not found] ` <1268848682.8335.5.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2010-03-17 18:08 ` Jeff Layton 2010-03-17 18:08 ` Jeff Layton 2010-01-25 22:15 ` [PATCH 12/12] NFS: Remove requirement for inode->i_mutex from nfs_invalidate_mapping Trond Myklebust 2010-01-25 22:15 ` Trond Myklebust 2010-01-25 22:15 ` [PATCH 11/12] NFS: Clean up nfs_sync_mapping Trond Myklebust
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1264547859.3788.37.camel@localhost \ --to=trond.myklebust-hgovqubeegtqt0dzr+alfa@public.gmane.org \ --cc=arjan-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org \ --cc=fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \ --cc=hch-jcswGhMUV9g@public.gmane.org \ --cc=hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org \ --cc=jack-AlSwsSmVLrQ@public.gmane.org \ --cc=jens.axboe-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org \ --cc=linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=mingo-X9Un+BFzKDI@public.gmane.org \ --cc=peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org \ --cc=sar-a+KepyhlMvJWk0Htik3J/w@public.gmane.org \ --cc=staubach-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \ --cc=viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.