From: Yongqiang Yang <xiaoqiangnk@gmail.com>
To: Andreas Dilger <adilger@dilger.ca>
Cc: Curt Wohlgemuth <curtw@google.com>,
linux-ext4@vger.kernel.org, jim@meyering.net, cmm@us.ibm.com,
hughd@google.com, tytso@mit.edu
Subject: Re: [PATCH v3] ext4: Don't set PageUptodate in ext4_end_bio()
Date: Tue, 26 Apr 2011 15:41:47 +0800 [thread overview]
Message-ID: <BANLkTikJjHKchRTt3t2jW+O_1wba9v8jmg@mail.gmail.com> (raw)
In-Reply-To: <4194C4D6-BE86-42CA-BBB4-A8A0E7E94EAC@dilger.ca>
This function is only called from write path which flushes pages to
disk, actually, pages' state have been set right at time when
write_end() is called. Why did we handle pages' state in this
function?
On Tue, Apr 26, 2011 at 6:40 AM, Andreas Dilger <adilger@dilger.ca> wrote:
> On 2011-04-25, at 2:23 PM, Curt Wohlgemuth wrote:
>> In the bio completion routine, we should not be setting
>> PageUptodate at all -- it's set at sys_write() time, and is
>> unaffected by success/failure of the write to disk.
>>
>> This can cause a page corruption bug when
>>
>> block size < page size
>>
>> @@ -203,46 +203,29 @@ static void ext4_end_bio(struct bio *bio, int error)
>> - /*
>> - * If this is a partial write which happened to make
>> - * all buffers uptodate then we can optimize away a
>> - * bogus readpage() for the next read(). Here we
>> - * 'discover' whether the page went uptodate as a
>> - * result of this (potentially partial) write.
>> - */
>> - if (!partial_write)
>> - SetPageUptodate(page);
>> -
>
> I think this is the important part of the code - if there is a read-after-write for a file that was written in "blocksize" units (blocksize < pagesize), does the page get set uptodate when all of the blocks have been written and/or the writing is at EOF? Otherwise, a read-after-write will always cause data to be fetched from disk needlessly, even though the uptodate information is already in cache.
>
> Cheers, Andreas
>
>
>
>
>
> --
> 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
>
--
Best Wishes
Yongqiang Yang
--
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
next prev parent reply other threads:[~2011-04-26 7:41 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-25 20:23 [PATCH v3] ext4: Don't set PageUptodate in ext4_end_bio() Curt Wohlgemuth
2011-04-25 22:40 ` Andreas Dilger
2011-04-25 22:45 ` Curt Wohlgemuth
2011-04-25 23:20 ` Curt Wohlgemuth
2011-04-26 0:58 ` Andreas Dilger
2011-04-26 4:32 ` Curt Wohlgemuth
2011-04-26 6:59 ` Yongqiang Yang
2011-04-26 15:37 ` Curt Wohlgemuth
2011-04-26 15:52 ` Yongqiang Yang
2011-04-26 7:41 ` Yongqiang Yang [this message]
2011-04-26 12:19 ` Ted Ts'o
2011-05-10 17:41 ` Hugh Dickins
2011-05-10 19:17 ` Ted Ts'o
2011-05-10 19:45 ` Hugh Dickins
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=BANLkTikJjHKchRTt3t2jW+O_1wba9v8jmg@mail.gmail.com \
--to=xiaoqiangnk@gmail.com \
--cc=adilger@dilger.ca \
--cc=cmm@us.ibm.com \
--cc=curtw@google.com \
--cc=hughd@google.com \
--cc=jim@meyering.net \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).