[PATCH] ext4: Fix up range_end offset in range cyclic writeback

[PATCH] ext4: Fix up range_end offset in range cyclic writeback

[Shantanu Goel]

Hi,

The patch below fixes the range_end calculation which should be a byte offset and not a page index.  Also, the patch updates the `end' index as well when looping again in range cyclic mode.

Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>

--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2324,9 +2324,10 @@ retry:
        blk_finish_plug(&plug);
        if (!io_done && !cycled) {
                cycled = 1;
+               end = index - 1;
                index = 0;
-               wbc->range_start = index << PAGE_CACHE_SHIFT;
-               wbc->range_end  = mapping->writeback_index - 1;
+               wbc->range_start = 0;
+               wbc->range_end  = ((loff_t)(end + 1) << PAGE_CACHE_SHIFT) - 1;
                goto retry;
        }

Re: [PATCH] ext4: Fix up range_end offset in range cyclic writeback

[Ted Ts'o]

On Sat, Jan 07, 2012 at 07:23:32PM -0800, Shantanu Goel wrote:
> 
> 
> The patch below fixes the range_end calculation which should be a
> byte offset and not a page index.  Also, the patch updates the `end'
> index as well when looping again in range cyclic mode.

Hi Shantanu,

This patch looks good; thanks for spotting this!  Can I ask how you
spotted this and whether it was by code inspection because the
incorrect code was causing some serious problem?

Basically, it's late in the merge window, and I'd rather not include
any new patches unless they are fixing a serious problem at this
point.  Otherwise, I'll make sure it gets merged for the next merge
window.

Thanks,

						- Ted

Re: [PATCH] ext4: Fix up range_end offset in range cyclic writeback

[Shantanu Goel]

[Resending as vger didn't like the HTML message so apologies for the duplicate Ted]


Hi Ted,

Thanks for confirming the fix is good.  We noticed that kupdate writebacks were happening at 
irregular intervals and started inspecting the VM writeback code which 
seemed to be doing its job correctly, and were eventually led to the 
ext4 writeback code.  Without the fix, it was sometimes take a very long time for a file to be flushed to disk asynchronously though synchronous flushes worked fine.  It would be nice if the fix could be included in 
3.3 release cycle.


Regards,Shantanu



________________________________
From: Ted Ts'o <tytso@mit.edu>
To: Shantanu Goel <sgoel01@yahoo.com> 
Cc: Kernel <linux-kernel@vger.kernel.org> 
Sent: Monday, January 9, 2012 10:17 AM
Subject: Re: [PATCH] ext4: Fix up range_end offset in range cyclic writeback

On Sat, Jan 07, 2012 at 07:23:32PM -0800, Shantanu Goel wrote:
> 
> 
> The patch below fixes the range_end calculation which should be a
> byte offset and not a page index.  Also, the patch updates the `end'
> index as well when looping again in range cyclic mode.

Hi Shantanu,

This patch looks good; thanks for spotting this!  Can I ask how you
spotted this and whether it was by code inspection because the
incorrect code was causing some serious problem?

Basically, it's late in the merge window, and I'd rather not include
any new patches unless they are fixing a serious problem at this
point.  Otherwise, I'll make sure it gets merged for the next merge
window.

Thanks,

                        - Ted

Re: [PATCH] ext4: Fix up range_end offset in range cyclic writeback

[Ted Ts'o]

On Mon, Jan 09, 2012 at 07:46:28AM -0800, Shantanu Goel wrote:
> 
> Thanks for confirming the fix is good.  We noticed that kupdate
> writebacks were happening at irregular intervals

So this was a performance issue that led you to this investigation?
How much of a performance issue are we talking about?

If it's a correctness issue that might lead to data loss because
fsync() wasn't doing its jobs, that's obviously higher priority than a
performance issue.

Again, I just wanted to characterize this correctly in the commit ---
BTW, this is good stuff to include in the commit description so in the
future, developers trying to go through the history can understand why
a particular patch is important (perhaps to backport into an
enterprise distro release, etc.)

Thanks, regards,

						- Ted

Re: [PATCH] ext4: Fix up range_end offset in range cyclic writeback

[Shantanu Goel]

> So this was a performance issue that led you to this investigation?
> How much of a performance issue are we talking about?

It's a performance issue in the sense that background flush of the file can take much longer (we observed > 5 mins in some instances) than the 30 second kupdate interval.

> If it's a correctness issue that might lead to data loss because
> fsync() wasn't doing its jobs, that's obviously higher priority than a
> performance issue.

fsync() is not affected and works correctly with or without the patch since it does not use the range_cyclic writeback mode, AFAICT.  The only "correctness" issue would be if an application relies on kupdate which uses range_cyclic writeback to periodically flush the file as governed by the /proc/sys/vm/dirty_writeback_centisecs tunable.  We have some applications which lost more data than they expected due to the writeback not happening at expected intervals.  One could argue the bug is in the application since it could have guaranteed the file was flushed by using fsync hence my quotes around correctness.  Unfortunately, modifying the application is not an easy option for us to pursue at this time.  We just wish to have the patch merged upstream and the exact timing is not that critical.

> Again, I just wanted to characterize this correctly in the commit ---
> BTW, this is good stuff to include in the commit description so in the
> future, developers trying to go through the history can understand why
> a particular patch is important (perhaps to backport into an
> enterprise distro release, etc.)

Sure, will do in future.

Thanks,
Shantanu