RE: [PATCH] hfsplus: limit sb_maxbytes to partition size

[Viacheslav Dubeyko]

On Fri, 2026-03-06 at 11:05 +0900, Hyunchul Lee wrote:
> On Fri, Mar 06, 2026 at 01:23:16AM +0000, Viacheslav Dubeyko wrote:
> > On Fri, 2026-03-06 at 09:57 +0900, Hyunchul Lee wrote:
> > > On Thu, Mar 05, 2026 at 11:21:19PM +0000, Viacheslav Dubeyko wrote:
> > > > On Thu, 2026-03-05 at 10:52 +0900, Hyunchul Lee wrote:
> > > > > > > 
> > > > > > > Sorry it's generic/285, not generic/268.
> > > > > > > in generic/285, there is a test that creates a hole exceeding the block
> > > > > > > size and appends small data to the file. hfsplus fails because it fills
> > > > > > > the block device and returns ENOSPC. However if it returns EFBIG
> > > > > > > instead, the test is skipped.
> > > > > > > 
> > > > > > > For writes like xfs_io -c "pwrite 8t 512", should fops->write_iter
> > > > > > > returns ENOSPC, or would it be better to return EFBIG?
> > > > > > > > 
> > > > > > 
> > > > > > Current hfsplus_file_extend() implementation doesn't support holes. I assume you
> > > > > > mean this code [1]:
> > > > > > 
> > > > > >         len = hip->clump_blocks;
> > > > > >         start = hfsplus_block_allocate(sb, sbi->total_blocks, goal, &len);
> > > > > >         if (start >= sbi->total_blocks) {
> > > > > >                 start = hfsplus_block_allocate(sb, goal, 0, &len);
> > > > > >                 if (start >= goal) {
> > > > > >                         res = -ENOSPC;
> > > > > >                         goto out;
> > > > > >                 }
> > > > > >         }
> > > > > > 
> > > > > > Am I correct?
> > > > > > 
> > > > > Yes,
> > > > > 
> > > > > hfsplus_write_begin()
> > > > >   cont_write_begin()
> > > > >     cont_expand_zero()
> > > > > 
> > > > > 1) xfs_io -c "pwrite 8t 512"
> > > > > 2) hfsplus_begin_write() is called with offset 2^43 and length 512
> > > > > 3) cont_expand_zero() allocates and zeroes out one block repeatedly
> > > > > for the range
> > > > > 0 to 2^43 - 1. To achieve this, hfsplus_write_begin() is called repeatedly.
> > > > > 4) hfsplus_write_begin() allocates one block through hfsplus_get_block() =>
> > > > > hfsplus_file_extend()
> > > > 
> > > > I think we can consider these directions:
> > > > 
> > > > (1) Currently, HFS+ code doesn't support holes. So, it means that
> > > > hfsplus_write_begin() can check pos variable and i_size_read(inode). If pos is
> > > > bigger than i_size_read(inode), then hfsplus_file_extend() will reject such
> > > > request. So, we can return error code (probably, -EFBIG) for this case without
> > > > calling hfsplus_file_extend(). But, from another point of view, maybe,
> > > > hfsplus_file_extend() could be one place for this check. Does it make sense?
> > > > 
> > > > (2) I think that hfsplus_file_extend() could treat hole or absence of free
> > > > blocks like -ENOSPC. Probably, we can change the error code from -ENOSPC to -
> > > > EFBIG in hfsplus_write_begin(). What do you think?
> > > > 
> > > Even if holes are not supported, shouldn't the following writes be
> > > supported?
> > > 
> > > xfs_io -f -c "pwrite 4k 512" <file-path>
> > > 
> > > If so, since we need to support cases where pos > i_size_read(inode),
> > 
> > The pos > i_size_read(inode) means that you create the hole. Because,
> 
> That's correct. However I believe that not supporting writes like the
> one mentioned above is a significant limitation. Filesystems that don't
> support sparse files, such as exFAT, allocate blocks and fill them with
> zeros.
> 

You are welcomed to write the code for HFS/HFS+. :) I'll be happy to see such
support.

> > oppositely, when HFS+ logic tries to allocate new block, then it expects to have
> > pos == i_size_read(inode). And we need to take into account this code [1]:
> > 
> > 	if (iblock >= hip->fs_blocks) {
> > 		if (!create)
> > 			return 0;
> > 		if (iblock > hip->fs_blocks) <-- This is the rejection of hole
> > 			return -EIO;
> > 		if (ablock >= hip->alloc_blocks) {
> > 			res = hfsplus_file_extend(inode, false);
> > 			if (res)
> > 				return res;
> > 		}
> > 	}
> > 
> > The generic_write_end() changes the inode size: i_size_write(inode, pos +
> > copied).
> 
> I think that it's not problem.
> 
> hfsplus_write_begin()
>   cont_write_begin()
>     cont_expand_zero()
> 
> cont_expand_zero() calls hfsplus_get_block() to allocate blocks between
> i_size_read(inode) and pos, if pos > i_size_read(inode).
> 

Currently, HFS/HFS+ expect that file should be extended without holes. It means
that next allocating block should be equal to number of allocated blocks in
file. If pos > i_size_read(inode), then it means that next allocating block is
not equal to number of allocated blocks in file.

If you imply that requested length could include multiple blocks for allocation,
then next allocating block should be equal to number of allocated blocks on
every step. And if the next allocating block is bigger than number of allocated
blocks in file, then hole creation is requested.

So, what are we discussing here? :)

> > 
> > > wouldn't the condition "pos - i_size_read(inode) > free space" be better?
> > > Also instead of checking every time in hfsplus_write_begin() or
> > > hfsplus_file_extend(), how about implementing the check in the
> > > file_operations->write_iter callback function, and returing EFBIG?
> > 
> > Which callback do you mean here? I am not sure that it's good idea.
> > 
> 
> Here is a simple code snippet.
> 
>  static const struct file_operations hfsplus_file_operations = {
> ...
> -       .write_iter     = generic_file_write_iter,
> +       .write_iter     = hfsplus_file_write_iter,
> ...
> 
> +ssize_t hfsplus_file_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> +{
> ...
> +       // check iocb->ki_pos is beyond i_size
> +
> +       ret = generic_file_write_iter(iocb, iter);
> 

The hfsplus_write_begin() will be called before hfsplus_file_write_iter() if we
are trying to extend the file. And hfsplus_get_block() calls
hfsplus_file_extend() that will call hfsplus_block_allocate(). So, everything
will happen before hfsplus_file_write_iter() call. What's the point to have the
check here?

Thanks,
Slava.

>