* ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path
@ 2011-01-31 20:44 Curt Wohlgemuth
2011-02-01 0:15 ` Curt Wohlgemuth
0 siblings, 1 reply; 5+ messages in thread
From: Curt Wohlgemuth @ 2011-01-31 20:44 UTC (permalink / raw)
To: ext4 development
The ext4 code path to bundle multiple pages for writeback in
ext4_bio_write_page() had a bug: we should be clearing buffer head dirty
flags before we submit the bio, not in the completion routine.
Also don't deference the bio in ext4_end_bio() after the bio_put() call.
(I also added a blank line in ext4_bio_write_page(), 'cause my eyes
constantly confused the complex 'for' conditions from the first statement
in the block.)
Signed-off-by: Curt Wohlgemuth <curtw@google.com>
---
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 6e0e99b..c3d490f 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -193,6 +193,7 @@ static void ext4_end_bio(struct bio *bio, int error)
struct inode *inode;
unsigned long flags;
int i;
+ sector_t bi_sector = bio->bi_sector;
BUG_ON(!io_end);
bio->bi_private = NULL;
@@ -210,9 +211,7 @@ static void ext4_end_bio(struct bio *bio, int error)
if (error)
SetPageError(page);
BUG_ON(!head);
- if (head->b_size == PAGE_CACHE_SIZE)
- clear_buffer_dirty(head);
- else {
+ if (head->b_size != PAGE_CACHE_SIZE) {
loff_t offset;
loff_t io_end_offset = io_end->offset + io_end->size;
@@ -224,7 +223,6 @@ static void ext4_end_bio(struct bio *bio, int error)
if (error)
buffer_io_error(bh);
- clear_buffer_dirty(bh);
}
if (buffer_delay(bh))
partial_write = 1;
@@ -260,7 +258,7 @@ static void ext4_end_bio(struct bio *bio, int error)
(unsigned long long) io_end->offset,
(long) io_end->size,
(unsigned long long)
- bio->bi_sector >> (inode->i_blkbits - 9));
+ bi_sector >> (inode->i_blkbits - 9));
}
/* Add the io_end to per-inode completed io list*/
@@ -383,6 +381,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
blocksize = 1 << inode->i_blkbits;
+ BUG_ON(!PageLocked(page));
BUG_ON(PageWriteback(page));
set_page_writeback(page);
ClearPageError(page);
@@ -400,12 +399,14 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
for (bh = head = page_buffers(page), block_start = 0;
bh != head || !block_start;
block_start = block_end, bh = bh->b_this_page) {
+
block_end = block_start + blocksize;
if (block_start >= len) {
clear_buffer_dirty(bh);
set_buffer_uptodate(bh);
continue;
}
+ clear_buffer_dirty(bh);
ret = io_submit_add_bh(io, io_page, inode, wbc, bh);
if (ret) {
/*
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path
2011-01-31 20:44 ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path Curt Wohlgemuth
@ 2011-02-01 0:15 ` Curt Wohlgemuth
2011-02-01 4:21 ` Eric Sandeen
0 siblings, 1 reply; 5+ messages in thread
From: Curt Wohlgemuth @ 2011-02-01 0:15 UTC (permalink / raw)
To: ext4 development
Sorry: I forgot to mention that I tested this on 2.6.37 under KVM
with the postgresql script specified by Ted in
1449032be17abb69116dbc393f67ceb8bd034f92 -- without this commit, and
hence by default using the multiblock writepages submittal code.
Without the patch, I'd hit the corruption problem about 50-70% of the
time. With the patch, I executed the script > 100 times with no
corruption seen.
Curt
On Mon, Jan 31, 2011 at 12:44 PM, Curt Wohlgemuth <curtw@google.com> wrote:
> The ext4 code path to bundle multiple pages for writeback in
> ext4_bio_write_page() had a bug: we should be clearing buffer head dirty
> flags before we submit the bio, not in the completion routine.
>
> Also don't deference the bio in ext4_end_bio() after the bio_put() call.
>
> (I also added a blank line in ext4_bio_write_page(), 'cause my eyes
> constantly confused the complex 'for' conditions from the first statement
> in the block.)
>
> Signed-off-by: Curt Wohlgemuth <curtw@google.com>
>
> ---
>
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 6e0e99b..c3d490f 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -193,6 +193,7 @@ static void ext4_end_bio(struct bio *bio, int error)
> struct inode *inode;
> unsigned long flags;
> int i;
> + sector_t bi_sector = bio->bi_sector;
>
> BUG_ON(!io_end);
> bio->bi_private = NULL;
> @@ -210,9 +211,7 @@ static void ext4_end_bio(struct bio *bio, int error)
> if (error)
> SetPageError(page);
> BUG_ON(!head);
> - if (head->b_size == PAGE_CACHE_SIZE)
> - clear_buffer_dirty(head);
> - else {
> + if (head->b_size != PAGE_CACHE_SIZE) {
> loff_t offset;
> loff_t io_end_offset = io_end->offset + io_end->size;
>
> @@ -224,7 +223,6 @@ static void ext4_end_bio(struct bio *bio, int error)
> if (error)
> buffer_io_error(bh);
>
> - clear_buffer_dirty(bh);
> }
> if (buffer_delay(bh))
> partial_write = 1;
> @@ -260,7 +258,7 @@ static void ext4_end_bio(struct bio *bio, int error)
> (unsigned long long) io_end->offset,
> (long) io_end->size,
> (unsigned long long)
> - bio->bi_sector >> (inode->i_blkbits - 9));
> + bi_sector >> (inode->i_blkbits - 9));
> }
>
> /* Add the io_end to per-inode completed io list*/
> @@ -383,6 +381,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>
> blocksize = 1 << inode->i_blkbits;
>
> + BUG_ON(!PageLocked(page));
> BUG_ON(PageWriteback(page));
> set_page_writeback(page);
> ClearPageError(page);
> @@ -400,12 +399,14 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
> for (bh = head = page_buffers(page), block_start = 0;
> bh != head || !block_start;
> block_start = block_end, bh = bh->b_this_page) {
> +
> block_end = block_start + blocksize;
> if (block_start >= len) {
> clear_buffer_dirty(bh);
> set_buffer_uptodate(bh);
> continue;
> }
> + clear_buffer_dirty(bh);
> ret = io_submit_add_bh(io, io_page, inode, wbc, bh);
> if (ret) {
> /*
>
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path
2011-02-01 0:15 ` Curt Wohlgemuth
@ 2011-02-01 4:21 ` Eric Sandeen
2011-02-01 7:37 ` Theodore Tso
0 siblings, 1 reply; 5+ messages in thread
From: Eric Sandeen @ 2011-02-01 4:21 UTC (permalink / raw)
To: Curt Wohlgemuth; +Cc: ext4 development
On 1/31/11 6:15 PM, Curt Wohlgemuth wrote:
> Sorry: I forgot to mention that I tested this on 2.6.37 under KVM
> with the postgresql script specified by Ted in
> 1449032be17abb69116dbc393f67ceb8bd034f92 -- without this commit, and
> hence by default using the multiblock writepages submittal code.
>
> Without the patch, I'd hit the corruption problem about 50-70% of the
> time. With the patch, I executed the script > 100 times with no
> corruption seen.
Can you resubmit with a changelog that indicates it's actually a corruption
fix? Maybe even in the summary. That is important information to have
for the commit...
-Eric
> Curt
>
> On Mon, Jan 31, 2011 at 12:44 PM, Curt Wohlgemuth <curtw@google.com> wrote:
>> The ext4 code path to bundle multiple pages for writeback in
>> ext4_bio_write_page() had a bug: we should be clearing buffer head dirty
>> flags before we submit the bio, not in the completion routine.
>>
>> Also don't deference the bio in ext4_end_bio() after the bio_put() call.
>>
>> (I also added a blank line in ext4_bio_write_page(), 'cause my eyes
>> constantly confused the complex 'for' conditions from the first statement
>> in the block.)
>>
>> Signed-off-by: Curt Wohlgemuth <curtw@google.com>
>>
>> ---
>>
>> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
>> index 6e0e99b..c3d490f 100644
>> --- a/fs/ext4/page-io.c
>> +++ b/fs/ext4/page-io.c
>> @@ -193,6 +193,7 @@ static void ext4_end_bio(struct bio *bio, int error)
>> struct inode *inode;
>> unsigned long flags;
>> int i;
>> + sector_t bi_sector = bio->bi_sector;
>>
>> BUG_ON(!io_end);
>> bio->bi_private = NULL;
>> @@ -210,9 +211,7 @@ static void ext4_end_bio(struct bio *bio, int error)
>> if (error)
>> SetPageError(page);
>> BUG_ON(!head);
>> - if (head->b_size == PAGE_CACHE_SIZE)
>> - clear_buffer_dirty(head);
>> - else {
>> + if (head->b_size != PAGE_CACHE_SIZE) {
>> loff_t offset;
>> loff_t io_end_offset = io_end->offset + io_end->size;
>>
>> @@ -224,7 +223,6 @@ static void ext4_end_bio(struct bio *bio, int error)
>> if (error)
>> buffer_io_error(bh);
>>
>> - clear_buffer_dirty(bh);
>> }
>> if (buffer_delay(bh))
>> partial_write = 1;
>> @@ -260,7 +258,7 @@ static void ext4_end_bio(struct bio *bio, int error)
>> (unsigned long long) io_end->offset,
>> (long) io_end->size,
>> (unsigned long long)
>> - bio->bi_sector >> (inode->i_blkbits - 9));
>> + bi_sector >> (inode->i_blkbits - 9));
>> }
>>
>> /* Add the io_end to per-inode completed io list*/
>> @@ -383,6 +381,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>>
>> blocksize = 1 << inode->i_blkbits;
>>
>> + BUG_ON(!PageLocked(page));
>> BUG_ON(PageWriteback(page));
>> set_page_writeback(page);
>> ClearPageError(page);
>> @@ -400,12 +399,14 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>> for (bh = head = page_buffers(page), block_start = 0;
>> bh != head || !block_start;
>> block_start = block_end, bh = bh->b_this_page) {
>> +
>> block_end = block_start + blocksize;
>> if (block_start >= len) {
>> clear_buffer_dirty(bh);
>> set_buffer_uptodate(bh);
>> continue;
>> }
>> + clear_buffer_dirty(bh);
>> ret = io_submit_add_bh(io, io_page, inode, wbc, bh);
>> if (ret) {
>> /*
>>
> --
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path
2011-02-01 4:21 ` Eric Sandeen
@ 2011-02-01 7:37 ` Theodore Tso
2011-02-02 16:58 ` Eric Sandeen
0 siblings, 1 reply; 5+ messages in thread
From: Theodore Tso @ 2011-02-01 7:37 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Curt Wohlgemuth, ext4 development
On Jan 31, 2011, at 11:21 PM, Eric Sandeen wrote:
> On 1/31/11 6:15 PM, Curt Wohlgemuth wrote:
>> Sorry: I forgot to mention that I tested this on 2.6.37 under KVM
>> with the postgresql script specified by Ted in
>> 1449032be17abb69116dbc393f67ceb8bd034f92 -- without this commit, and
>> hence by default using the multiblock writepages submittal code.
>>
>> Without the patch, I'd hit the corruption problem about 50-70% of the
>> time. With the patch, I executed the script > 100 times with no
>> corruption seen.
>
> Can you resubmit with a changelog that indicates it's actually a corruption
> fix? Maybe even in the summary. That is important information to have
> for the commit...
Agreed, although I'd also point out that the corruption fix was for code that
was disabled by default before 2.6.37 shipped, precisely because of the
reported corruption bug, and that this code should make it safe to use
the mblk_io_submit mount option, which will become the default in
2.6.39.
Normally I add such clarifications in the commit description (I'm used to
editing descriptions to correct English and to make things clearer, so for
me this is no big deal), but if you want resubmit with an updated description
and save me some work, I won't complain. :-)
-- Ted
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path
2011-02-01 7:37 ` Theodore Tso
@ 2011-02-02 16:58 ` Eric Sandeen
0 siblings, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2011-02-02 16:58 UTC (permalink / raw)
To: Theodore Tso; +Cc: Curt Wohlgemuth, ext4 development
On 2/1/11 1:37 AM, Theodore Tso wrote:
>
> On Jan 31, 2011, at 11:21 PM, Eric Sandeen wrote:
>
>> On 1/31/11 6:15 PM, Curt Wohlgemuth wrote:
>>> Sorry: I forgot to mention that I tested this on 2.6.37 under KVM
>>> with the postgresql script specified by Ted in
>>> 1449032be17abb69116dbc393f67ceb8bd034f92 -- without this commit, and
>>> hence by default using the multiblock writepages submittal code.
>>>
>>> Without the patch, I'd hit the corruption problem about 50-70% of the
>>> time. With the patch, I executed the script > 100 times with no
>>> corruption seen.
>>
>> Can you resubmit with a changelog that indicates it's actually a corruption
>> fix? Maybe even in the summary. That is important information to have
>> for the commit...
>
> Agreed, although I'd also point out that the corruption fix was for code that
> was disabled by default before 2.6.37 shipped, precisely because of the
> reported corruption bug, and that this code should make it safe to use
> the mblk_io_submit mount option, which will become the default in
> 2.6.39.
Right, but a corruption fix should never be committed w/o clear notice.
Speaking as one who does a fair bit of git archaeology, it drives me nuts
to have commits which look innocuous and are in fact momentous.
> Normally I add such clarifications in the commit description (I'm used to
> editing descriptions to correct English and to make things clearer, so for
> me this is no big deal), but if you want resubmit with an updated description
> and save me some work, I won't complain. :-)
Ok, just trying to coach a bit ;)
-Eric
> -- Ted
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-02-02 16:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-31 20:44 ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path Curt Wohlgemuth
2011-02-01 0:15 ` Curt Wohlgemuth
2011-02-01 4:21 ` Eric Sandeen
2011-02-01 7:37 ` Theodore Tso
2011-02-02 16:58 ` Eric Sandeen
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).