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

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Mon, 2026-08-31 at 14:21 +0100, Pedro Falcato wrote:
> 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)
> 

The problem here that before modification of block bitmap's page we
need to be sure that writeback operation has been finished. Otherwise,
we could have finally inconsistent state of the block bitmap on the
volume. Frankly speaking, I don't quite follow what are we discussing
here? Do you have a particular suggestion or improvement of the patch 3
in the series? What is the wrong in patch 3, from your point of view?

Thanks,
Slava.

> > 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.