From patchwork Fri Feb 28 08:37:10 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kara X-Patchwork-Id: 325060 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 19E612C009F for ; Fri, 28 Feb 2014 19:37:14 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751224AbaB1IhN (ORCPT ); Fri, 28 Feb 2014 03:37:13 -0500 Received: from cantor2.suse.de ([195.135.220.15]:53825 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750927AbaB1IhM (ORCPT ); Fri, 28 Feb 2014 03:37:12 -0500 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 2887A75023; Fri, 28 Feb 2014 08:37:11 +0000 (UTC) Received: by quack.suse.cz (Postfix, from userid 1000) id 3BAEE80D1E; Fri, 28 Feb 2014 09:37:10 +0100 (CET) Date: Fri, 28 Feb 2014 09:37:10 +0100 From: Jan Kara To: ZhangZhen Cc: jack@suse.cz, linux-ext4@vger.kernel.org Subject: Re: [PATCH] ext3: fix some wrong comments Message-ID: <20140228083710.GC28074@quack.suse.cz> References: <53100429.3000205@huawei.com> <53103ABB.9030100@huawei.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <53103ABB.9030100@huawei.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Fri 28-02-14 15:28:59, ZhangZhen wrote: > The comments in the code are wrong, because every generic_file_write() > is replaced by generic_file_aio_write(). > > Signed-off-by: Zhang Zhen > --- > fs/ext3/inode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c > index 384b6eb..0b94832 100644 > --- a/fs/ext3/inode.c > +++ b/fs/ext3/inode.c > @@ -1574,7 +1574,7 @@ static int buffer_unmapped(handle_t *handle, struct buffer_head *bh) > * > * Similar for: > * > - * ext3_file_write() -> generic_file_write() -> __alloc_pages() -> ... > + * ext3_file_write() -> generic_file_aio_write() -> __alloc_pages() -> ... Well, but ext3_file_write() doesn't exist either so the comment obviously needs further updates. > * > * Same applies to ext3_get_block(). We will deadlock on various things like > * lock_journal and i_truncate_mutex. > @@ -3212,7 +3212,7 @@ out_brelse: > * > * We are called from a few places: > * > - * - Within generic_file_write() for O_SYNC files. > + * - Within generic_file_aio_write() for O_SYNC files. > * Here, there will be no transaction running. We wait for any running > * transaction to commit. > * This is correct but as I'm looking through the comment it needs much larger update. So something like attached patches looks more appropriate. Thanks for pointing me to those outdated comments. Honza From ff75d02aa0fabc7379677b7ede181acb0204c77d Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Fri, 28 Feb 2014 09:31:10 +0100 Subject: [PATCH 2/2] ext3: Update outdated comment before ext3_ordered_writepage() The comment is heavily outdated. The recursion into the filesystem isn't possible because we use GFP_NOFS for our allocations, the issue about block_write_full_page() dirtying tail page is long resolved as well (that function doesn't dirty buffers at all), and finally we don't start a transaction if all blocks are already allocated and mapped. Signed-off-by: Jan Kara --- fs/ext3/inode.c | 47 ++++------------------------------------------- 1 file changed, 4 insertions(+), 43 deletions(-) diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c index 2fef98abb207..4ecf88fb69a8 100644 --- a/fs/ext3/inode.c +++ b/fs/ext3/inode.c @@ -1559,56 +1559,17 @@ static int buffer_unmapped(handle_t *handle, struct buffer_head *bh) } /* - * Note that we always start a transaction even if we're not journalling - * data. This is to preserve ordering: any hole instantiation within - * __block_write_full_page -> ext3_get_block() should be journalled - * along with the data so we don't crash and then get metadata which + * Note that whenever we need to map blocks we start a transaction even if + * we're not journalling data. This is to preserve ordering: any hole + * instantiation within __block_write_full_page -> ext3_get_block() should be + * journalled along with the data so we don't crash and then get metadata which * refers to old data. * * In all journalling modes block_write_full_page() will start the I/O. * - * Problem: - * - * ext3_writepage() -> kmalloc() -> __alloc_pages() -> page_launder() -> - * ext3_writepage() - * - * Similar for: - * - * ext3_file_write() -> generic_file_write() -> __alloc_pages() -> ... - * - * Same applies to ext3_get_block(). We will deadlock on various things like - * lock_journal and i_truncate_mutex. - * - * Setting PF_MEMALLOC here doesn't work - too many internal memory - * allocations fail. - * - * 16May01: If we're reentered then journal_current_handle() will be - * non-zero. We simply *return*. - * - * 1 July 2001: @@@ FIXME: - * In journalled data mode, a data buffer may be metadata against the - * current transaction. But the same file is part of a shared mapping - * and someone does a writepage() on it. - * - * We will move the buffer onto the async_data list, but *after* it has - * been dirtied. So there's a small window where we have dirty data on - * BJ_Metadata. - * - * Note that this only applies to the last partial page in the file. The - * bit which block_write_full_page() uses prepare/commit for. (That's - * broken code anyway: it's wrong for msync()). - * - * It's a rare case: affects the final partial page, for journalled data - * where the file is subject to bith write() and writepage() in the same - * transction. To fix it we'll need a custom block_write_full_page(). - * We'll probably need that anyway for journalling writepage() output. - * * We don't honour synchronous mounts for writepage(). That would be * disastrous. Any write() or metadata operation will sync the fs for * us. - * - * AKPM2: if all the page's buffers are mapped to disk and !data=journal, - * we don't need to open a transaction here. */ static int ext3_ordered_writepage(struct page *page, struct writeback_control *wbc) -- 1.8.1.4