Re: [PATCH] xfs: fix s_max_bytes to MAX_LFS_FILESIZE if needed

[Jeff Liu <jeff.liu@oracle.com>]

Hi Dave,

Thanks for the quick response.

On 07/10/2013 02:48 PM, Dave Chinner wrote:

> On Wed, Jul 10, 2013 at 02:28:20PM +0800, Jeff Liu wrote:
>> Could anyone help to review this patch?
> 
> Sorry, I missed it.
> 
>>> On 32-bit machine, the s_maxbytes is larger than the MAX_LFS_FILESIZE limits if CONFIG_LBDAF is
>>> not enabled.  Hence it's possible to create a huge file via buffered-IO write with a given offset
>>> beyond this limitation. e.g.
>>>
>>> # block_size=4096
>>> # offset=$(((2**32 - 1) * $block_size))
>>> # xfs_io -f -c "pwrite $offset $block_size" /storage/test_file
>>>
>>> In this case, xfs_io will hang at the page writeback stage soon since the given offset would
>>> cause an overflow at xfs_vm_writepage():
>>>
>>> end_index = offset >> PAGE_CACHE_SHIFT;
>>> last_index = (offset - 1) >> PAGE_CACHE_SHIFT;
>>> if (page->index >= end_index) {
>>>                 unsigned offset_into_page = offset & (PAGE_CACHE_SIZE - 1);
>>>
>>>                 /*
>>>                  * Just skip the page if it is fully outside i_size, e.g. due
>>>                  * to a truncate operation that is in progress.
>>>                  */
>>>                 if (page->index >= end_index + 1 || offset_into_page == 0) {
>>> 		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>                         unlock_page(page);
>>>                         return 0;
>>>                 }
>>> end_index is unsigned long so that the max value is '2^32-1 = 4294967295', and it
>>> would be evaluated to the max value with the given offset(when writing the page offset
>>> up to s_max_bytes) for above test case.  As a result, (page->index >= end_index + 1) is
>>> ok as (end_index + 1) is overflowed to ZERO.
>>>
>>> Actually, create a file as above on 32-bit machine should be failed with EFBIG error returned
>>> because there has strict check up at generic_write_checks() against the given offset with a
>>> *correct* s_max_bytes.
>>>
>>> This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater
>>> than it.
> 
> Isn't MAX_LFS_FILESIZE defined on 32 bit systems to 8TB and the
> problem here is that we are overflowing at 16TB? If so, that means
> addin gthis patch will potentially cause problems with existing
> working setups that have (sparse) files larger than 8TB on 32 bit
> systems.

Yes, but maybe I should say end_index is wrapped to zero rather than
overflow in this situation.

> 
> So, can't we simply subtract PAGE_CACHE_SIZE from the offset
> being returned to avoid this overflow?

It seems that this change does not works to me because page->index is
greater than end_index in most cases for large files.
IOWs, what I mentioned in comment log is incorrect and it misled you.

I worked out another patch, it looks works for this test but I need to
run some extra tests before posting.

Thanks,
-Jeff


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs