Re: [PATCH v2] ext4: check if offset+length is valid in fallocate

[Tadeusz Struk <tadeusz.struk@linaro.org>]

On 3/31/22 07:43, Theodore Ts'o wrote:
> On Tue, Mar 15, 2022 at 02:54:39PM -0700, Tadeusz Struk wrote:
>> @@ -3967,6 +3968,16 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
>>   		   offset;
>>   	}
>>   
>> +	/*
>> +	 * For punch hole the length + offset needs to be at least within
>> +	 * one block before last
>> +	 */
>> +	max_length = sbi->s_bitmap_maxbytes - inode->i_sb->s_blocksize;
>> +	if (offset + length >= max_length) {
>> +		ret = -ENOSPC;
>> +		goto out_mutex;
>> +	}
> 
> I wonder if we would be better off just simply capping length to
> max_length?  If length is set to some large value, such as LONG_MAX,
> it's pretty clear what the intention should be, which is to simply do
> the equivalent of truncating the file at offset.  Perhaps we should
> just do that?

Don't think that would be the correct behavior. ftrucnate (or truncate)
modify the file size, but fallocate with FALLOC_FL_PUNCH_HOLE should not.

man 2 fallocate says:
"...
The FALLOC_FL_PUNCH_HOLE flag must be ORed with FALLOC_FL_KEEP_SIZE in mode;
in other words, even when punching off the end of the file, the file size
(as reported by stat(2)) does not change.
"
that is enforced by vfs:
https://elixir.bootlin.com/linux/v5.17.1/source/fs/open.c#L245

> 
> That being said, we should be consistent with what other file systems
> do when they are asked to punch a hole starting at offset and
> extending out to LONG_MAX.

For all the supported file systems, apart from ext4, only btrfs, gfs2, and xfs
support fallocate and FALLOC_FL_PUNCH_HOLE mode.
Looking at what they do is they round the length of the space to be freed
i.e. offset + length to valid value and then perform the operation
using the valid values.

https://elixir.bootlin.com/linux/v5.17.1/source/fs/gfs2/bmap.c#L2424
https://elixir.bootlin.com/linux/v5.17.1/source/fs/btrfs/file.c#L2506

For ext4 this would mean that one could only deallocate space up to
the one before last block. I will change this to do the same in the
next version if that's ok with you.

> 
> Also, if we are going to return an error, I don't think ENOSPC is the
> correct error to be returning.

I took it from man 2 fallocate, my first suspicion was that it crashed
because the disk on my VM wasn't big enough. It was a bad choice.

-- 
Thanks,
Tadeusz