Redfs ubuntu hwe writeback switch to dio#187
Conversation
Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
3b7eec3 to
ec8fc13
Compare
A FUSE_NOTIFY_INVAL_INODE data invalidation means another (remote) entity
is modifying the file.
Rather than react to a single notify, keep a per-inode moving average of
how fast data invalidations arrive for the whole file: an EWMA of the
inter-arrival interval, updated under fi->lock on every notify
(fuse_notify_inval_hot()). The inode is latched only once the average
spacing drops below an internal threshold (FUSE_NOTIFY_DIO_INTERVAL) while
a local writer is open; a lone or occasional notify keeps the average high
and does not trip the switch. The heuristic has no external knob -- its
parameters (EWMA weight, threshold, seed) are source-level constants.
Introduce the forced-direct-IO latch (FUSE_I_FORCE_DIO):
- fuse_reverse_inval_inode() folds each data invalidation into the moving
average and sets the latch when it trips with a local writer present;
- fuse_file_{read,write}_iter() and fuse_cache_write_iter() route to the
direct path while latched; fuse_dio_{wr_exclusive_lock,lock,unlock}()
use the shared parallel-dio path and bypass the cached/uncached
accounting;
- fuse_file_io_open() opens new files uncached so they do not re-enter
caching mode;
- fuse_prepare_release() clears the latch once the last writer is gone
and fuse_file_release() drops any clean folios a racing read
repopulated; fuse_file_mmap() reverts to caching mode (a mapping needs
the page cache).
Latching to direct IO is only coherent if no buffered write can deposit
dirty folios into the page cache after it has been dropped. Add a
per-inode rw_semaphore, wb_inval_rwsem, to serialise the buffered-write
page-cache dirtying against the latch transition. The writeback path
holds it for read around the dirtying and re-checks the latch under it;
fuse_reverse_inval_inode() holds it for write around its invalidate +
latch set. The notification may be delivered by the same server thread
that still owes a reply to an in-flight write holding the inode lock, so
it takes the rwsem with a trylock and never blocks: if the writer has gone
it skips the latch and only invalidates the notified range. The writer's
read-side section stays free of server round-trips because under fc->dlm
the partial-write RMW read is skipped.
Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
commit 0c58a97 ("fuse: remove tmp folio for writebacks and internal rb tree") removed temp folios for dirty page writeback. Consequently, fuse can now use the default writeback accounting. With switching fuse to use default writeback accounting, there are some added benefits. This updates wb->writeback_inodes tracking as well now and updates writeback throughput estimates after writeback completion. This commit also removes inc_wb_stat() and dec_wb_stat(). These have no callers anymore now that fuse does not call them. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> Reviewed-by: David Hildenbrand <david@redhat.com> Reviewed-by: Bernd Schubert <bschubert@ddn.com> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> (cherry picked from commit 494d2f5)
fuse_readahead() batches whole folios into a single request, capped at min(fc->max_pages, fc->max_read/PAGE_SIZE) pages, but fuse_init_file_inode() let the page cache build folios up to MAX_PAGECACHE_ORDER. A large sequential read could thus produce a folio bigger than one request can carry: the first loop iteration took the folio_pages > cur_pages path, fired WARN_ON(!pages), and broke with ap->num_folios == 0. fuse_send_readpages() was still called and dereferenced a NULL ap->folios[0] via folio_pos(), oopsing at CR2=0x20 (folio->index). Cap the folio order to the per-request page limit so the page cache can never build an unserviceable folio. There is a patch for upstream that does exactly the same. In later versions this is not needed but for 6.17 we need this. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
ec8fc13 to
8f359ed
Compare
| * Only regular files initialise it -- it shares storage | ||
| * with the readdir-cache union arm. | ||
| */ | ||
| struct rw_semaphore wb_inval_rwsem; |
There was a problem hiding this comment.
Horst, this lock can also be used for the serialization between the cached read/write I/Os and invalidation, that is the AOP_TRUNCATED_PAGE we discussed before, so this lock perhaps is not only used for this DIO switch purpose.
There was a problem hiding this comment.
If we use this patch ... I don't think that that case will happen again exactly for the reason you mention.
There was a problem hiding this comment.
Good, however, I did see the synchronization between the invalidation and write, but didn't see where the synchronization between the read and invalidation is? That is it seems the cached read path doesn't hold this lock to synchronize with invalidation?
There was a problem hiding this comment.
That is true. Maybe I have to think about that ...
There was a problem hiding this comment.
I think we should do that in another PR ... this is already too large as it is
There was a problem hiding this comment.
So just to clarify that: the serialization missing part between the invalidation and read will be done in another PR? Also for the invalidation and write serialization, I think still there's a window: once the write gets permission to run, one invalidation could be just done and cleared the DLM lock, so the write path should check whether the DLM cache is still there before moving forward.
There was a problem hiding this comment.
to your first question ... yes. But due to a technical problem. If we make such heavy use of this synchronization for READ and WRITE I changed the data type (percpu_rw_semaphore) for one that is supposedly be faster in the light path. That leads to a way bigger patch.
Extending FOPEN_PARALLEL_DIRECT_WRITES writes were forced onto the
exclusive inode lock, re-serializing the parallel phase. The exclusive
lock only bundled "write + advance i_size + undo-on-failure" into one
unit. But i_size is committed by fuse_write_update_attr() under
fi->lock, only on a successful growing write and independent of the
inode rwsem -- so shared-lock writers commit size correctly and have
nothing to undo. Drop the past-EOF exclusive triggers and gate the
whole-file fuse_do_truncate() rollback on holding the exclusive lock.
Lock mode is passed to __fuse_direct_IO(); i_size is committed at the
same point in every path, only the failure rollback differs:
non-exclusive (relaxed, parallel):
fuse_direct_write_iter
fuse_dio_lock -> inode_lock_shared (exclusive=false)
__fuse_direct_IO(.., false)
fuse_direct_io() write to server
fuse_write_update_attr() commit i_size (on success)
no rollback
exclusive (append / caching / !parallel):
fuse_direct_write_iter
fuse_dio_lock -> inode_lock (exclusive=true)
__fuse_direct_IO(.., true)
fuse_direct_io() write to server
fuse_write_update_attr() commit i_size (on success)
ret<0 & extend -> fuse_do_truncate() rollback
exclusive (caching-mode O_DIRECT):
fuse_cache_write_iter -> inode_lock (exclusive)
generic_file_direct_write -> fuse_direct_IO
__fuse_direct_IO(.., true)
fuse_direct_io() write to server
fuse_write_update_attr() commit i_size (on success)
ret<0 & extend -> fuse_do_truncate() rollback
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
fuse_dio_lock() takes an uncached_io reference (via fuse_inode_uncached_io_start()) only when FUSE_I_FORCE_DIO is clear, while fuse_dio_unlock() decided whether to drop it by re-reading FUSE_I_FORCE_DIO. On this tree the latch is toggled asynchronously by the inode-invalidation notify-storm path, so the bit can differ between the lock and the unlock of a single direct write: - clear at lock (reference taken), set before unlock: the reference is never dropped, leaving fi->iocachectr permanently negative and hanging the next caching-mode open; - set at lock (no reference), cleared before unlock: fuse_inode_uncached_io_end() is called without a matching start, tripping WARN_ON(fi->iocachectr >= 0) and corrupting the counter. Record in fuse_dio_lock() whether a reference was actually taken and have fuse_dio_unlock() drop it based on that captured decision instead of re-testing the racy bit, so the accounting stays balanced regardless of any FORCE_DIO transition mid-write. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
c02ef5c to
1c52882
Compare
|
@hazhou-ddn @achhenderson so do you guys think we can merge this? |
9d6feaa
into
DDNStorage:redfs-ubuntu-hwe-6.17.0-16.16-24.04.1
No description provided.