Patchwork ext3: fix some wrong comments

login
register
mail settings
Submitter Jan Kara
Date Feb. 28, 2014, 8:37 a.m.
Message ID <20140228083710.GC28074@quack.suse.cz>
Download mbox | patch
Permalink /patch/325060/
State Not Applicable
Headers show

Comments

Jan Kara - Feb. 28, 2014, 8:37 a.m.
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 <zhenzhang.zhang@huawei.com>
> ---
>  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
ZhangZhen - March 3, 2014, 1:44 a.m.
On 2014/2/28 16:37, Jan Kara wrote:
> 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 <zhenzhang.zhang@huawei.com>
>> ---
>>  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
> 
Hi Honza,
	
Your fix is more better than mine. It's a good chance for me to learn, thanks!

								Zhen

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

From ff75d02aa0fabc7379677b7ede181acb0204c77d Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
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 <jack@suse.cz>
---
 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