Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755153AbZGOO2U (ORCPT ); Wed, 15 Jul 2009 10:28:20 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755066AbZGOO2T (ORCPT ); Wed, 15 Jul 2009 10:28:19 -0400 Received: from mga03.intel.com ([143.182.124.21]:50708 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754869AbZGOO2S (ORCPT ); Wed, 15 Jul 2009 10:28:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.42,404,1243839600"; d="scan'208";a="165289450" Date: Wed, 15 Jul 2009 22:28:06 +0800 From: Wu Fengguang To: Hugh Dickins Cc: Andrew Morton , LKML , "linux-fsdevel@vger.kernel.org" , Andi Kleen Subject: Re: [PATCH] shmem: call set_page_dirty() with locked page Message-ID: <20090715142806.GA18892@localhost> References: <20090714092926.GA23969@localhost> <20090715111012.GA1403@localhost> <20090715140447.GA15177@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3845 Lines: 101 On Wed, Jul 15, 2009 at 10:22:46PM +0800, Hugh Dickins wrote: > On Wed, 15 Jul 2009, Wu Fengguang wrote: > > shmem: call set_page_dirty() with locked page > > > > The dirtying of page and set_page_dirty() can be moved into the page lock. > > > > - In shmem_write_end(), the page was dirtied while the page lock was held, > > but it's being marked dirty just after dropping the page lock. > > - In shmem_symlink(), both dirtying and marking can be moved into page lock. > > > > It's valuable for the hwpoison code to know whether one bad page can be dropped > > without losing data. It mainly judges by testing the PG_dirty bit after taking > > the page lock. So it becomes important that the dirtying of page and the > > marking of dirtiness are both done inside the page lock. Which is a common > > practice, but sadly not a rule. > > > > The noticeable exceptions are > > - mapped pages > > - pages with buffer_heads > > The above pages could go dirty at any time. Fortunately the hwpoison will > > unmap the page and release the buffer_heads beforehand anyway. > > > > Many other types of pages (eg. metadata pages) can also be dirtied at will by > > their owners, the hwpoison code cannot do meaningful things to them anyway. > > Only the dirtiness of pagecache pages owned by regular files are interested. > > > > CC: Hugh Dickins > > Ah, those were the days... ;) Ah yeah :) > > Signed-off-by: Wu Fengguang > > Acked-by: Hugh Dickins Thanks for you kind review! --- shmem: call set_page_dirty() with locked page The dirtying of page and set_page_dirty() can be moved into the page lock. - In shmem_write_end(), the page was dirtied while the page lock was held, but it's being marked dirty just after dropping the page lock. - In shmem_symlink(), both dirtying and marking can be moved into page lock. It's valuable for the hwpoison code to know whether one bad page can be dropped without losing data. It mainly judges by testing the PG_dirty bit after taking the page lock. So it becomes important that the dirtying of page and the marking of dirtiness are both done inside the page lock. Which is a common practice, but sadly not a rule. The noticeable exceptions are - mapped pages - pages with buffer_heads The above pages could go dirty at any time. Fortunately the hwpoison will unmap the page and release the buffer_heads beforehand anyway. Many other types of pages (eg. metadata pages) can also be dirtied at will by their owners, the hwpoison code cannot do meaningful things to them anyway. Only the dirtiness of pagecache pages owned by regular files are interested. Acked-by: Hugh Dickins Signed-off-by: Wu Fengguang --- mm/shmem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- linux.orig/mm/shmem.c +++ linux/mm/shmem.c @@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc if (pos + copied > inode->i_size) i_size_write(inode, pos + copied); - unlock_page(page); set_page_dirty(page); + unlock_page(page); page_cache_release(page); return copied; @@ -1968,13 +1968,13 @@ static int shmem_symlink(struct inode *d iput(inode); return error; } - unlock_page(page); inode->i_mapping->a_ops = &shmem_aops; inode->i_op = &shmem_symlink_inode_operations; kaddr = kmap_atomic(page, KM_USER0); memcpy(kaddr, symname, len); kunmap_atomic(kaddr, KM_USER0); set_page_dirty(page); + unlock_page(page); page_cache_release(page); } if (dir->i_mode & S_ISGID) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/