On 26.09.19 14:59, Brian Foster wrote:
> On Thu, Sep 26, 2019 at 12:57:49PM +0200, Max Reitz wrote:
>> Hi,
>>
>> I’ve noticed that fallocating some range on XFS sometimes does not
>> include the last block covered by the range, when the start offset is
>> unaligned.
>>
>> (Tested on 5.3.0-gf41def397.)
>>
>> This happens whenever ceil((offset + len) / block_size) - floor(offset /
>> block_size) > ceil(len / block_size), for example:
>>
>> Let block_size be 4096.  Then (on XFS):
>>
>> $ fallocate -o 2048 -l 4096 foo   # Range [2048, 6144)
>> $ xfs_bmap foo
>> foo:
>>         0: [0..7]: 80..87
>>         1: [8..15]: hole
>>
>> There should not be a hole there.  Both of the first two blocks should
>> be allocated.  XFS will do that if I just let the range start one byte
>> sooner and increase the length by one byte:
>>
>> $ rm -f foo
>> $ fallocate -o 2047 -l 4097 foo   # Range [2047, 6144)
>> $ xfs_bmap foo
>> foo:
>>         0: [0..15]: 88..103
>>
>>
>> (See [1] for a more extensive reasoning why this is a bug.)
>>
>>
>> The problem is (as far as I can see) that xfs_alloc_file_space() rounds
>> count (which equals len) independently of the offset.  So in the
>> examples above, 4096 is rounded to one block and 4097 is rounded to two;
>> even though the first example actually touches two blocks because of the
>> misaligned offset.
>>
>> Therefore, this should fix the problem (and does fix it for me):
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 0910cb75b..4f4437030 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -864,6 +864,7 @@ xfs_alloc_file_space(
>>  	xfs_filblks_t		allocatesize_fsb;
>>  	xfs_extlen_t		extsz, temp;
>>  	xfs_fileoff_t		startoffset_fsb;
>> +	xfs_fileoff_t		endoffset_fsb;
>>  	int			nimaps;
>>  	int			quota_flag;
>>  	int			rt;
>> @@ -891,7 +892,8 @@ xfs_alloc_file_space(
>>  	imapp = &imaps[0];
>>  	nimaps = 1;
>>  	startoffset_fsb	= XFS_B_TO_FSBT(mp, offset);
>> -	allocatesize_fsb = XFS_B_TO_FSB(mp, count);
>> +	endoffset_fsb = XFS_B_TO_FSB(mp, offset + count);
>> +	allocatesize_fsb = endoffset_fsb - startoffset_fsb;
>>
>>  	/*
>>  	 * Allocate file space until done or until there is an error
>>
> 
> That looks like a reasonable fix to me and it's in the spirit of how
> xfs_free_file_space() works as well (outside of the obvious difference
> in how unaligned boundary blocks are handled). Care to send a proper
> patch?

I’ve never sent a kernel patch before, but I’ll give it a go.

Max