From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932715AbXAQUGY (ORCPT ); Wed, 17 Jan 2007 15:06:24 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S932719AbXAQUGY (ORCPT ); Wed, 17 Jan 2007 15:06:24 -0500 Received: from omx1-ext.sgi.com ([192.48.179.11]:58203 "EHLO omx1.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932715AbXAQUGX (ORCPT ); Wed, 17 Jan 2007 15:06:23 -0500 Date: Wed, 17 Jan 2007 12:05:53 -0800 (PST) From: Christoph Lameter To: Peter Zijlstra cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Trond Myklebust Subject: Re: [PATCH] nfs: fix congestion control In-Reply-To: <1168985323.5975.53.camel@lappy> Message-ID: References: <20070116054743.15358.77287.sendpatchset@schroedinger.engr.sgi.com> <20070116135325.3441f62b.akpm@osdl.org> <1168985323.5975.53.camel@lappy> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 16 Jan 2007, Peter Zijlstra wrote: > The current NFS client congestion logic is severely broken, it marks the > backing device congested during each nfs_writepages() call and implements > its own waitqueue. This is the magic bullet that Andrew is looking for to fix the NFS issues? > Index: linux-2.6-git/include/linux/nfs_fs_sb.h > =================================================================== > --- linux-2.6-git.orig/include/linux/nfs_fs_sb.h 2007-01-12 08:03:47.000000000 +0100 > +++ linux-2.6-git/include/linux/nfs_fs_sb.h 2007-01-12 08:53:26.000000000 +0100 > @@ -82,6 +82,8 @@ struct nfs_server { > struct rpc_clnt * client_acl; /* ACL RPC client handle */ > struct nfs_iostats * io_stats; /* I/O statistics */ > struct backing_dev_info backing_dev_info; > + atomic_t writeback; /* number of writeback pages */ > + atomic_t commit; /* number of commit pages */ > int flags; /* various flags */ I think writeback is frequently incremented? Would it be possible to avoid a single global instance of an atomic_t here? In a busy NFS system with lots of processors writing via NFS this may cause a hot cacheline that limits write speed. Would it be possible to use NR_WRITEBACK? If not then maybe add another ZVC counter named NFS_NFS_WRITEBACK? > Index: linux-2.6-git/mm/page-writeback.c > =================================================================== > --- linux-2.6-git.orig/mm/page-writeback.c 2007-01-12 08:03:47.000000000 +0100 > +++ linux-2.6-git/mm/page-writeback.c 2007-01-12 08:53:26.000000000 +0100 > @@ -167,6 +167,12 @@ get_dirty_limits(long *pbackground, long > *pdirty = dirty; > } > > +int dirty_pages_exceeded(struct address_space *mapping) > +{ > + return dirty_exceeded; > +} > +EXPORT_SYMBOL_GPL(dirty_pages_exceeded); > + Export the variable instead of adding a new function? Why does it take an address space parameter that is not used? > Index: linux-2.6-git/fs/inode.c > =================================================================== > --- linux-2.6-git.orig/fs/inode.c 2007-01-12 08:03:47.000000000 +0100 > +++ linux-2.6-git/fs/inode.c 2007-01-12 08:53:26.000000000 +0100 > @@ -81,6 +81,7 @@ static struct hlist_head *inode_hashtabl > * the i_state of an inode while it is in use.. > */ > DEFINE_SPINLOCK(inode_lock); > +EXPORT_SYMBOL_GPL(inode_lock); Hmmm... Commits to all NFS servers will be globally serialized via the inode_lock? From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Wed, 17 Jan 2007 12:05:53 -0800 (PST) From: Christoph Lameter Subject: Re: [PATCH] nfs: fix congestion control In-Reply-To: <1168985323.5975.53.camel@lappy> Message-ID: References: <20070116054743.15358.77287.sendpatchset@schroedinger.engr.sgi.com> <20070116135325.3441f62b.akpm@osdl.org> <1168985323.5975.53.camel@lappy> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-linux-mm@kvack.org Return-Path: To: Peter Zijlstra Cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Trond Myklebust List-ID: On Tue, 16 Jan 2007, Peter Zijlstra wrote: > The current NFS client congestion logic is severely broken, it marks the > backing device congested during each nfs_writepages() call and implements > its own waitqueue. This is the magic bullet that Andrew is looking for to fix the NFS issues? > Index: linux-2.6-git/include/linux/nfs_fs_sb.h > =================================================================== > --- linux-2.6-git.orig/include/linux/nfs_fs_sb.h 2007-01-12 08:03:47.000000000 +0100 > +++ linux-2.6-git/include/linux/nfs_fs_sb.h 2007-01-12 08:53:26.000000000 +0100 > @@ -82,6 +82,8 @@ struct nfs_server { > struct rpc_clnt * client_acl; /* ACL RPC client handle */ > struct nfs_iostats * io_stats; /* I/O statistics */ > struct backing_dev_info backing_dev_info; > + atomic_t writeback; /* number of writeback pages */ > + atomic_t commit; /* number of commit pages */ > int flags; /* various flags */ I think writeback is frequently incremented? Would it be possible to avoid a single global instance of an atomic_t here? In a busy NFS system with lots of processors writing via NFS this may cause a hot cacheline that limits write speed. Would it be possible to use NR_WRITEBACK? If not then maybe add another ZVC counter named NFS_NFS_WRITEBACK? > Index: linux-2.6-git/mm/page-writeback.c > =================================================================== > --- linux-2.6-git.orig/mm/page-writeback.c 2007-01-12 08:03:47.000000000 +0100 > +++ linux-2.6-git/mm/page-writeback.c 2007-01-12 08:53:26.000000000 +0100 > @@ -167,6 +167,12 @@ get_dirty_limits(long *pbackground, long > *pdirty = dirty; > } > > +int dirty_pages_exceeded(struct address_space *mapping) > +{ > + return dirty_exceeded; > +} > +EXPORT_SYMBOL_GPL(dirty_pages_exceeded); > + Export the variable instead of adding a new function? Why does it take an address space parameter that is not used? > Index: linux-2.6-git/fs/inode.c > =================================================================== > --- linux-2.6-git.orig/fs/inode.c 2007-01-12 08:03:47.000000000 +0100 > +++ linux-2.6-git/fs/inode.c 2007-01-12 08:53:26.000000000 +0100 > @@ -81,6 +81,7 @@ static struct hlist_head *inode_hashtabl > * the i_state of an inode while it is in use.. > */ > DEFINE_SPINLOCK(inode_lock); > +EXPORT_SYMBOL_GPL(inode_lock); Hmmm... Commits to all NFS servers will be globally serialized via the inode_lock? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org