lkml.org 
[lkml]   [2022]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] ext4: check if offset+length is valid in fallocate
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

\
 
 \ /
  Last update: 2022-03-31 20:04    [W:0.154 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site