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 <[email protected]>
---
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) {
/*
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 <[email protected]> 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 <[email protected]>
>
> ---
>
> 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) {
> ? ? ? ? ? ? ? ? ? ? ? ?/*
>
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 <[email protected]> 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 <[email protected]>
>>
>> ---
>>
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
>