Re: [PATCH v2 0/7] hfsplus: convert regular file I/O to iomap-based operations

[Pedro Falcato <pfalcato@suse.de>]

On Fri, Aug 28, 2026 at 02:52:22PM -0700, Viacheslav Dubeyko wrote:
> On Thu, 2026-08-27 at 20:35 +0100, Pedro Falcato wrote:
> > On Thu, Aug 27, 2026 at 11:35:12AM -0700, Viacheslav Dubeyko wrote:
> > > On Thu, 2026-08-27 at 01:06 +0100, Matthew Wilcox wrote:
> > > > On Wed, Aug 26, 2026 at 03:56:07PM -0700, Viacheslav Dubeyko
> > > > wrote:
> > > > > Christoph Hellwig has detected that taking the page lock around
> > > > > the kmap/modify/kunmap section is not enough on its own:
> > > > > writeback drops the page lock before the write actually
> > > > > completes,
> > > > > so a mutator that only waits on the lock can still start
> > > > > rewriting
> > > > > a page whose old contents are still in flight to the device.
> > > > > Mark the allocation file's mapping with
> > > > > mapping_set_stable_writes()
> > > > > and call folio_wait_stable() right after taking the page lock
> > > > > in
> > > > > both functions, so a mutator also waits out any writeback that
> > > > > was
> > > > > already in progress when it acquired the lock.
> > > > 
> > > > Why would you indirect through the stable mechanism rather than
> > > > just
> > > > calling folio_wait_writeback() directly?
> > > 
> > > The HFS+ allocation file (block bitmap) is represented by sbi-
> > > > alloc_file inode and mapping represents the block bitmap space.
> > > > If one
> > > thread is calling hfsplus_block_allocate() or hfsplus_block_free(),
> > > then it tries to modify the content of folios/pages in this
> > > mapping.
> > > But writeback could happen in the background in another thread. It
> > > sounds like "folio's contents to stay unchanged while writeback is
> > > in
> > > progress". This is why folio_wait_stable() was suggested. Do you
> > > mean
> > > that it is not exactly correct approach? Do you think that
> > 
> > Why do you want to do this? It's definitely unusual for filesystems
> > (AFAIK)?
> > It sounds like you're trying to guarantee some sort of consistency in
> > your
> > writes, without journaling, but I can't tell exactly why.
> > 
> > (FWIW, if you're trying to do this for metadata consistency reasons,
> > I
> > really don't think this works, because not only do you not know if
> > data
> > hits the disk, but you also don't know if other writes (e.g inodes)
> > hit
> > the disk, etc)
> 
> The sbi->alloc_file [1] is not regular inode. It is embedded into a
> superblock structure the special inode for representing metadata:
> 
> struct hfsplus_sb_info {
> <skipped>
> 	struct inode *alloc_file;
> <skipped>
> };
> 
> This inode participates in metadata operations only:
> hfsplus_block_allocate(), hfsplus_block_free(). If this inode is marked
> as dirty, then hfsplus_file_fsync() [2], hfsplus_sync_fs() [3] can
> call:
> 
>  filemap_write_and_wait(sbi->alloc_file->i_mapping)
> 
> And this call could take place concurrently with
> hfsplus_block_allocate(), hfsplus_block_free(). The main goal of
> folio_wait_stable() or folio_wait_writeback() is to prevent the
> allocate or free methods from accessing memory page until it under
> writeback.

Why is that a problem? Racing writeback is fine for most block devices
(those that aren't have bdev_stable_writes(), thus just doing folio_wait_stable()
should work, I think)

> writes.
> 
> But if you believe that the whole approach of managing Allocation File
> is implemented in wrong way many years ago, then you are welcomed top
> implement it in right way. :)

Oh. I don't think that's wrong. I'm just trying to understand why you think
excluding against writeback unconditionally is the right thing to do.

-- 
Pedro