Re: [PATCH v4 6/7] hfsplus: introduce iomap-based file_operations

[Viacheslav Dubeyko]

On Fri, 2026-09-18 at 15:39 +0200, Christoph Hellwig wrote:
> On Mon, Sep 14, 2026 at 04:39:40PM -0700, Viacheslav Dubeyko wrote:
> > This patch
> 
> Patch descriptions are not supposed to start with "this patch".
> By the time this fets into git, it isn't a patch any more :)

It depends on how we can defines a patch. From my point of view, patch
is a piece of code that can be applied on some state of git tree. But
even if this piece of code was adopted and applied into the git tree,
then it could be extracted as patch again. So, it exists as patch
forever in the git tree because it is incremental change of the tree.

> 
> > @@ -55,7 +58,6 @@ int hfsplus_file_fsync(struct file *file, loff_t
> > start, loff_t end,
> >  	error = file_write_and_wait_range(file, start, end);
> >  	if (error)
> >  		return error;
> > -	inode_lock(inode);
> >  
> >  	/*
> >  	 * Sync inode metadata into the catalog and extent trees.
> > @@ -114,15 +116,184 @@ int hfsplus_file_fsync(struct file *file,
> > loff_t start, loff_t end,
> >  	if (!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
> >  		blkdev_issue_flush(inode->i_sb->s_bdev);
> >  
> > +	return error;
> > +}
> 
> Changing fsync locking feels like something that should move
> into a well-documented prep patch.

Yeah, as I remember, it was a fix of some issue. So, yes, it is
possible to have it as a patch. I need simply to remember what this fix
was about. :)

> 
> > +/*
> > + * hfsplus_fallback_buffered_write() - fall back to buffered I/O
> > for the
> 
> Pointless function name in the comment.
> 
> > + * tail of a write that iomap_dio_rw() could not perform directly
> > + * (unaligned tail, or no blocks could be mapped without
> > allocation
> > + * outside the direct path).
> > + */
> > +static ssize_t hfsplus_fallback_buffered_write(struct kiocb *iocb,
> > +						struct iov_iter
> > *from)
> > +{
> 
> Can this use direct_write_fallback()?

Yes, I think it makes sense.

> 
> > +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
> > +	loff_t total_capacity;
> > +	ssize_t ret;
> > +	int err;
> > +
> > +	inode_lock(inode);
> > +
> > +	ret = generic_write_checks(iocb, iter);
> > +	if (ret <= 0)
> > +		goto unlock;
> > +
> > +	total_capacity = (loff_t)sbi->total_blocks << sbi-
> > >alloc_blksz_shift;
> > +	if (iocb->ki_pos >= total_capacity) {
> 
> This is weird.  The total capacity should not matter for a write.

Yes, I see your point. And I think you are right here. I am trying to
remember what was the point of this check. If I am not wrong it is the
artifact of some bug fix. I need to remove this code and double check
what will happen. Potentially, it could be obsolete code artifact.

Thanks,
Slava.