xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix s_max_bytes to MAX_LFS_FILESIZE if needed
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Wed, 10 Jul 2013 21:14:14 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>, "Michael L. Semon" <mlsemon35@xxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130710064855.GQ3438@dastard>
References: <5167E160.3020800@xxxxxxxxxx> <51DCFF04.9070606@xxxxxxxxxx> <20130710064855.GQ3438@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120410 Thunderbird/11.0.1
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


<Prev in Thread] Current Thread [Next in Thread>