From 7865483f165f88a6ad163ce9630d112f0815c84c Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Wed, 7 Oct 2026 14:38:57 -0700 Subject: [PATCH 1/6] Add scoutfs_lock_invalidate_pending() Add a quick (racey) function that tests whether there's an invalidation pending on a lock. Signed-off-by: Zach Brown --- kmod/src/lock.c | 13 +++++++++++++ kmod/src/lock.h | 1 + 2 files changed, 14 insertions(+) diff --git a/kmod/src/lock.c b/kmod/src/lock.c index f3b21345..f20f1f69 100644 --- a/kmod/src/lock.c +++ b/kmod/src/lock.c @@ -1450,6 +1450,19 @@ bool scoutfs_lock_protected(struct scoutfs_lock *lock, struct scoutfs_key *key, &lock->start, &lock->end) == 0; } +/* + * Returns true if there is an invalidation pending on the lock. + * + * This is racey and doesn't test modes. But it can still be useful for + * writers who will take non-destructive action if there's an + * invalidation pending that must necessarily be incompatible with their + * hold. + */ +bool scoutfs_lock_invalidate_pending(struct scoutfs_lock *lock) +{ + return lock->invalidate_pending; +} + void scoutfs_free_unused_locks(struct super_block *sb) { DECLARE_LOCK_INFO(sb, linfo); diff --git a/kmod/src/lock.h b/kmod/src/lock.h index 07908d62..54c8cedb 100644 --- a/kmod/src/lock.h +++ b/kmod/src/lock.h @@ -103,6 +103,7 @@ void scoutfs_lock_del_coverage(struct super_block *sb, struct scoutfs_lock_coverage *cov); bool scoutfs_lock_protected(struct scoutfs_lock *lock, struct scoutfs_key *key, enum scoutfs_lock_mode mode); +bool scoutfs_lock_invalidate_pending(struct scoutfs_lock *lock); u64 scoutfs_lock_ino_refresh_gen(struct super_block *sb, u64 ino); From 2e6ede38a9cd8d63511bd102a76ba39691612ad0 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Wed, 7 Oct 2026 14:39:37 -0700 Subject: [PATCH 2/6] Reduce cluster lock hold times while truncating Whole file truncate operations are relatively unique in that they can do a large amount of work under one cluster lock acquisition. Typically a large amount of work is done with many reasonably large operations (write, read, etc). When truncating a huge amount of extents under one cluster lock other users of the cluster lock across the network can be starved. While not strictly incorrect, it makes users like ls seemingly hang and is not a useful experience. This makes final inode deletion and releasing modify a file's extents under multiple cluster lock acquisitions. In both cases we only cycle the cluster lock if we see a pending invalidation after we've done a reasonable amount of work. In the release case this changes behavior by allowing large release calls to interleave. This is deemed acceptable because, in practice, userspace archive managers are strictly controlling how tasks release inodes and aren't relying on the ioctl to resolve concurrency. Signed-off-by: Zach Brown --- kmod/src/data.c | 67 +++++++++++++++------ kmod/src/data.h | 28 ++++++++- kmod/src/inode.c | 78 ++++++++++++++++-------- kmod/src/ioctl.c | 125 ++++++++++++++++++++++----------------- kmod/src/scoutfs_trace.h | 18 +++--- 5 files changed, 206 insertions(+), 110 deletions(-) diff --git a/kmod/src/data.c b/kmod/src/data.c index 44ca4c9d..290958b8 100644 --- a/kmod/src/data.c +++ b/kmod/src/data.c @@ -195,7 +195,7 @@ struct scoutfs_ext_ops data_ext_ops = { * - +iblock of next logical block to truncate the next block from */ static s64 truncate_extents(struct super_block *sb, struct inode *inode, - u64 ino, u64 iblock, u64 last, bool offline, + u64 ino, u64 iblock, u64 last, int dti, struct scoutfs_lock *lock) { DECLARE_DATA_INFO(sb, datinf); @@ -212,7 +212,7 @@ static s64 truncate_extents(struct super_block *sb, struct inode *inode, int err; int i; - flags = offline ? SEF_OFFLINE : 0; + flags = (dti & DTI_OFFLINE) ? SEF_OFFLINE : 0; ret = 0; for (i = 0; iblock <= last; i++) { @@ -236,7 +236,7 @@ static s64 truncate_extents(struct super_block *sb, struct inode *inode, } /* nothing to do when already offline and unmapped */ - if ((offline && (ext.flags & SEF_OFFLINE)) && !ext.map) { + if (((dti & DTI_OFFLINE) && (ext.flags & SEF_OFFLINE)) && !ext.map) { iblock = ext.start + ext.len; continue; } @@ -288,26 +288,32 @@ static s64 truncate_extents(struct super_block *sb, struct inode *inode, * Free blocks inside the logical block range from 'iblock' to 'last', * inclusive. * - * If 'offline' is given then blocks are freed an offline mapping is - * left behind. Only blocks that have been allocated can be marked - * offline. + * The DTI_ flags change behavior as described at their definition. * - * If the inode is provided then we update its tracking of the online - * and offline blocks. If it's not provided then the inode is being - * destroyed and isn't reachable, we don't need to update it. + * If the inode is provided then we update its item and the tracking of + * online and offline blocks. If it's not provided then the inode is + * being destroyed and isn't reachable, we don't need to update it. * - * The caller is in charge of locking the inode and data, but we may - * have to modify far more items than fit in a transaction so we're in - * charge of batching updates into transactions. If the inode is - * provided then we're responsible for updating its item as we go. + * The caller is in charge of locking the inode and data while we manage + * transactions. dti flags can have us return partial progress so the + * caller can drop locks. */ -int scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, - u64 ino, u64 iblock, u64 last, bool offline, +s64 scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, + u64 ino, u64 iblock, u64 last, int dti, struct scoutfs_lock *lock) { struct scoutfs_inode_info *si = NULL; LIST_HEAD(ind_locks); + u64 after_isize; + u64 offline; + u64 online; s64 ret = 0; + u64 seq; + + if (WARN_ON_ONCE((dti & (DTI_START_ISIZE|DTI_ONLINE_DONE)) && !inode)) { + ret = -EINVAL; + goto out; + } WARN_ON_ONCE(inode && !inode_is_locked(inode)); @@ -315,7 +321,22 @@ int scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, if (last > SCOUTFS_BLOCK_SM_MAX) last = SCOUTFS_BLOCK_SM_MAX; - trace_scoutfs_data_truncate_items(sb, iblock, last, offline); + if (dti & DTI_START_ISIZE) { + after_isize = (i_size_read(inode) + SCOUTFS_BLOCK_SM_SIZE - 1) + >> SCOUTFS_BLOCK_SM_SHIFT; + if (iblock < after_isize) + iblock = min(after_isize, last); + } + + if (dti & DTI_ONLINE_DONE) { + scoutfs_inode_get_onoff(inode, &online, &offline); + if (online > 0) { + ret = 0; + goto out; + } + } + + trace_scoutfs_data_truncate_items(sb, iblock, last, dti); if (WARN_ON_ONCE(last < iblock)) return -EINVAL; @@ -323,7 +344,16 @@ int scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, if (inode) si = SCOUTFS_I(inode); + if (dti & DTI_PARTIAL_INVALIDATE) + seq = scoutfs_trans_sample_seq(sb); + while (iblock <= last) { + if ((dti & DTI_PARTIAL_INVALIDATE) && iblock > 0 && + scoutfs_lock_invalidate_pending(lock) && scoutfs_trans_sample_seq(sb) != seq) { + ret = iblock; + break; + } + if (inode) ret = scoutfs_inode_index_lock_hold(inode, &ind_locks, true, false); else @@ -339,8 +369,7 @@ int scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, } if (ret == 0) - ret = truncate_extents(sb, inode, ino, iblock, last, - offline, lock); + ret = truncate_extents(sb, inode, ino, iblock, last, dti, lock); if (inode) scoutfs_update_inode_item(inode, lock, &ind_locks); @@ -356,7 +385,7 @@ int scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, iblock = ret; ret = 0; } - +out: return ret; } diff --git a/kmod/src/data.h b/kmod/src/data.h index d1e701ec..67d2eda9 100644 --- a/kmod/src/data.h +++ b/kmod/src/data.h @@ -66,8 +66,32 @@ int scoutfs_data_alloc_block(struct super_block *sb, struct inode *inode, int scoutfs_get_block_write(struct inode *inode, sector_t iblock, struct buffer_head *bh, int create); -int scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, - u64 ino, u64 iblock, u64 last, bool offline, +enum { + /* + * Set offline flag in extents when blocks are freed. + */ + DTI_OFFLINE = (1 << 0), + /* + * Set the starting block to the block after isize, only if the + * initial starting block is less. Requires a valid inode + * pointer. + */ + DTI_START_ISIZE = (1 << 1), + /* + * Stop and return 0 if there are any online blocks. Requires + * a valid inode pointer. + */ + DTI_ONLINE_DONE = (1 << 2), + /* + * Allow partial progress when there's an invalidation pending + * on the inode lock. Will always make at least a transaction's + * worth of progress truncating. If partial progress was made + * then the next block to resume truncating from is returned. + */ + DTI_PARTIAL_INVALIDATE = (1 << 3), +}; +s64 scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, + u64 ino, u64 iblock, u64 last, int dti, struct scoutfs_lock *lock); int scoutfs_data_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 start, u64 len); diff --git a/kmod/src/inode.c b/kmod/src/inode.c index 6db15554..614aa26b 100644 --- a/kmod/src/inode.c +++ b/kmod/src/inode.c @@ -460,8 +460,7 @@ int scoutfs_complete_truncate(struct inode *inode, struct scoutfs_lock *lock) start = (i_size_read(inode) + SCOUTFS_BLOCK_SM_SIZE - 1) >> SCOUTFS_BLOCK_SM_SHIFT; ret = scoutfs_data_truncate_items(inode->i_sb, inode, - scoutfs_ino(inode), start, ~0ULL, - false, lock); + scoutfs_ino(inode), start, U64_MAX, 0, lock); err = clear_truncate_flag(inode, lock); return ret ? ret : err; @@ -1635,16 +1634,20 @@ int scoutfs_inode_orphan_delete(struct super_block *sb, u64 ino, struct scoutfs_ } /* - * Remove all the items associated with a given inode. This is only - * called once nlink has dropped to zero and nothing has the inode open - * so we don't have to worry about dirents referencing the inode or link + * Remove the items associated with a given inode. This is only called + * once nlink has dropped to zero and nothing has the inode open so we + * don't have to worry about dirents referencing the inode or link * backrefs. Dropping nlink to 0 also created an orphan item. That * orphan item will continue triggering attempts to finish previous * partial deletion until all deletion is complete and the orphan item * is removed. + * + * Returns a positive block number if data extent removal made partial + * progress and could be continued from that block number. */ -static int delete_inode_items(struct super_block *sb, u64 ino, struct scoutfs_inode *sinode, - struct scoutfs_lock *lock, struct scoutfs_lock *orph_lock) +static s64 delete_inode_items(struct super_block *sb, u64 ino, u64 sblock, + struct scoutfs_inode *sinode, struct scoutfs_lock *lock, + struct scoutfs_lock *orph_lock) { struct scoutfs_key key; LIST_HEAD(ind_locks); @@ -1652,7 +1655,7 @@ static int delete_inode_items(struct super_block *sb, u64 ino, struct scoutfs_in umode_t mode; u64 ind_seq; u64 size; - int ret; + s64 ret; scoutfs_inode_init_key(&key, ino); @@ -1662,9 +1665,9 @@ static int delete_inode_items(struct super_block *sb, u64 ino, struct scoutfs_in /* remove data items in their own transactions */ if (S_ISREG(mode)) { - ret = scoutfs_data_truncate_items(sb, NULL, ino, 0, ~0ULL, - false, lock); - if (ret) + ret = scoutfs_data_truncate_items(sb, NULL, ino, sblock, U64_MAX, + DTI_PARTIAL_INVALIDATE, lock); + if (ret != 0) goto out; } @@ -1787,7 +1790,7 @@ out: } /* - * Try to delete all the items for an unused inode number. This is the + * Try to delete the items for an unused inode number. This is the * relatively slow path that uses cluster locks, network requests, and * IO to ensure correctness. Callers should try hard to avoid calling * when there's no work to do. @@ -1811,10 +1814,14 @@ out: * on a bit in the lock data so that we only have one deletion attempt * per inode under this mount's cluster lock. * - * Returns -EAGAIN if we either did some cleanup work or are unable to finish - * cleaning up this inode right now. + * Returns -EAGAIN if we either did some cleanup work or are unable to + * finish cleaning up this inode right now. + * + * This can make partial progress if it had to drop the inode lock while + * removing extent items. In this case it returns the postive starting + * data block number that truncate could resume from. */ -static int try_delete_inode_items(struct super_block *sb, u64 ino) +static s64 try_delete_inode_items(struct super_block *sb, u64 ino, u64 sblock) { struct inode_deletion_lock_data *ldata = NULL; struct scoutfs_lock *orph_lock = NULL; @@ -1824,7 +1831,7 @@ static int try_delete_inode_items(struct super_block *sb, u64 ino) bool clear_trying = false; u64 group_nr; int bit_nr; - int ret; + s64 ret; trace_scoutfs_try_delete(sb, ino); @@ -1873,7 +1880,7 @@ static int try_delete_inode_items(struct super_block *sb, u64 ino) if (ret < 0) goto out; - ret = delete_inode_items(sb, ino, &sinode, lock, orph_lock); + ret = delete_inode_items(sb, ino, sblock, &sinode, lock, orph_lock); if (ret == 0) { ret = -EAGAIN; scoutfs_inc_counter(sb, inode_deleted); @@ -1890,12 +1897,33 @@ out: } /* - * As we evicted an inode we need to decide to try and delete its items - * or not, which is expensive. We only try when we have lock coverage - * and the inode has been unlinked. This catches the common case of - * regular deletion so deletion will be performed in the final unlink - * task. It also catches open-unlink or o_tmpfile that aren't cached on - * other nodes. + * Keep deleting the inode's items as long as we make partial progress + * deleting extent items. + * + * Returns -EAGAIN if we either did some cleanup work or are unable to + * finish cleaning up this inode right now. + */ +static int try_delete_inode(struct super_block *sb, u64 ino) +{ + u64 sblock = 0; + s64 ret; + + do { + ret = try_delete_inode_items(sb, ino, sblock); + if (ret > 0) + sblock = ret; + } while (ret > 0); + + return ret; +} + +/* + * As we evict an inode we decide if we should delete its items or not, + * which is expensive. We only try when we have lock coverage and the + * inode has been unlinked. This catches the common case of regular + * deletion so deletion will be performed in the final unlink task. It + * also catches open-unlink or o_tmpfile that aren't cached on other + * nodes. * * Inodes being evicted outside of lock coverage, by referenced dentries * or inodes that survived the attempt to drop them as their lock was @@ -1918,7 +1946,7 @@ void scoutfs_evict_inode(struct inode *inode) scoutfs_omap_clear(sb, ino); if (scoutfs_lock_is_covered(sb, &si->ino_lock_cov) && inode->i_nlink == 0) - try_delete_inode_items(sb, scoutfs_ino(inode)); + try_delete_inode(sb, scoutfs_ino(inode)); } clear_inode(inode); @@ -2164,7 +2192,7 @@ static void inode_orphan_scan_worker(struct work_struct *work) scoutfs_inc_counter(sb, orphan_scan_attempts); trace_scoutfs_orphan_scan_work(sb, ino); - ret = try_delete_inode_items(sb, ino); + ret = try_delete_inode(sb, ino); if (ret == -EAGAIN) { work_todo = true; ret = 0; diff --git a/kmod/src/ioctl.c b/kmod/src/ioctl.c index 6da71d95..907e7d1d 100644 --- a/kmod/src/ioctl.c +++ b/kmod/src/ioctl.c @@ -298,6 +298,61 @@ out: return ret; } +static int release_range(struct super_block *sb, struct inode *inode, u64 data_version, + u64 sblock, u64 eblock, int dti) +{ + struct scoutfs_lock *lock = NULL; + bool unlock = false; + int ret; + + while (sblock <= eblock) { + + inode_lock(inode); + unlock = true; + + ret = scoutfs_lock_inode(sb, SCOUTFS_LOCK_WRITE, + SCOUTFS_LKF_REFRESH_INODE, inode, &lock); + if (ret < 0) + goto out; + + if (!S_ISREG(inode->i_mode)) { + ret = -EINVAL; + goto out; + } + + if (scoutfs_inode_data_version(inode) != data_version) { + ret = -ESTALE; + goto out; + } + + inode_dio_wait(inode); + + /* drop all clean and dirty cached blocks in the range */ + truncate_inode_pages_range(&inode->i_data, + sblock << SCOUTFS_BLOCK_SM_SHIFT, + ((eblock + 1) << SCOUTFS_BLOCK_SM_SHIFT) - 1); + + ret = scoutfs_data_truncate_items(sb, inode, scoutfs_ino(inode), sblock, eblock, + dti | DTI_PARTIAL_INVALIDATE, lock); + + scoutfs_unlock(sb, lock, SCOUTFS_LOCK_WRITE); + lock = NULL; + inode_unlock(inode); + unlock = false; + + if (ret <= 0) + goto out; + sblock = ret; + } + + ret = 0; +out: + scoutfs_unlock(sb, lock, SCOUTFS_LOCK_WRITE); + if (unlock) + inode_unlock(inode); + return ret; +} + /* * The caller has a version of the data available in the given byte * range in an external archive. As long as the data version still @@ -306,23 +361,23 @@ out: * recall from the archive. * * If the file's online blocks drop to 0 then we also truncate any - * blocks beyond i_size. This honors the intent of fully releasing a file - * without the user needing to know to release past i_size or truncate. + * blocks beyond i_size. This honors the intent of fully releasing a + * file without the user needing to know to release past i_size or + * truncate. * - * XXX permissions? - * XXX a lot of this could be generic file write prep + * The inode's cluster lock can be unlocked multiple times while the + * region is released. This allows concurrent release operations to + * interleave. Normal data plane operations that would change + * data_version will block while there are offline blocks so they likely + * won't be concurrent with release. */ static long scoutfs_ioc_release(struct file *file, unsigned long arg) { struct inode *inode = file_inode(file); struct super_block *sb = inode->i_sb; struct scoutfs_ioctl_release args; - struct scoutfs_lock *lock = NULL; u64 sblock; u64 eblock; - u64 online; - u64 offline; - u64 isize; __u64 tmp; int ret; @@ -338,60 +393,20 @@ static long scoutfs_ioc_release(struct file *file, unsigned long arg) (args.length & SCOUTFS_BLOCK_SM_MASK)) return -EINVAL; + if (!(file->f_mode & FMODE_WRITE)) + return -EINVAL; + ret = mnt_want_write_file(file); if (ret) return ret; - inode_lock(inode); - - ret = scoutfs_lock_inode(sb, SCOUTFS_LOCK_WRITE, - SCOUTFS_LKF_REFRESH_INODE, inode, &lock); - if (ret) - goto out; - - if (!S_ISREG(inode->i_mode)) { - ret = -EINVAL; - goto out; - } - - if (!(file->f_mode & FMODE_WRITE)) { - ret = -EINVAL; - goto out; - } - - if (scoutfs_inode_data_version(inode) != args.data_version) { - ret = -ESTALE; - goto out; - } - - inode_dio_wait(inode); - - /* drop all clean and dirty cached blocks in the range */ - truncate_inode_pages_range(&inode->i_data, args.offset, - args.offset + args.length - 1); - sblock = args.offset >> SCOUTFS_BLOCK_SM_SHIFT; eblock = (args.offset + args.length - 1) >> SCOUTFS_BLOCK_SM_SHIFT; - ret = scoutfs_data_truncate_items(sb, inode, scoutfs_ino(inode), - sblock, - eblock, true, - lock); - if (ret == 0) { - scoutfs_inode_get_onoff(inode, &online, &offline); - isize = i_size_read(inode); - if (online == 0 && isize) { - sblock = (isize + SCOUTFS_BLOCK_SM_SIZE - 1) - >> SCOUTFS_BLOCK_SM_SHIFT; - ret = scoutfs_data_truncate_items(sb, inode, - scoutfs_ino(inode), - sblock, U64_MAX, - false, lock); - } - } + ret = release_range(sb, inode, args.data_version, sblock, eblock, DTI_OFFLINE); + if (ret == 0) + ret = release_range(sb, inode, args.data_version, 0, U64_MAX, + DTI_START_ISIZE | DTI_ONLINE_DONE); -out: - scoutfs_unlock(sb, lock, SCOUTFS_LOCK_WRITE); - inode_unlock(inode); mnt_drop_write_file(file); trace_scoutfs_ioc_release_ret(sb, scoutfs_ino(inode), ret); diff --git a/kmod/src/scoutfs_trace.h b/kmod/src/scoutfs_trace.h index 90fbc0aa..3fa75315 100644 --- a/kmod/src/scoutfs_trace.h +++ b/kmod/src/scoutfs_trace.h @@ -378,26 +378,26 @@ DEFINE_EVENT(scoutfs_data_file_extent_class, scoutfs_data_fiemap_extent, ); TRACE_EVENT(scoutfs_data_truncate_items, - TP_PROTO(struct super_block *sb, __u64 iblock, __u64 last, int offline), + TP_PROTO(struct super_block *sb, __u64 iblock, __u64 last, int dti), - TP_ARGS(sb, iblock, last, offline), + TP_ARGS(sb, iblock, last, dti), TP_STRUCT__entry( SCSB_TRACE_FIELDS __field(__u64, iblock) __field(__u64, last) - __field(int, offline) + __field(int, dti) ), TP_fast_assign( SCSB_TRACE_ASSIGN(sb); __entry->iblock = iblock; __entry->last = last; - __entry->offline = offline; + __entry->dti = dti; ), - TP_printk(SCSBF" iblock %llu last %llu offline %u", SCSB_TRACE_ARGS, - __entry->iblock, __entry->last, __entry->offline) + TP_printk(SCSBF" iblock %llu last %llu dti %x", SCSB_TRACE_ARGS, + __entry->iblock, __entry->last, __entry->dti) ); TRACE_EVENT(scoutfs_data_wait_check, @@ -1211,7 +1211,7 @@ TRACE_EVENT(scoutfs_delete_inode, ); TRACE_EVENT(scoutfs_delete_inode_end, - TP_PROTO(struct super_block *sb, u64 ino, umode_t mode, u64 size, int ret), + TP_PROTO(struct super_block *sb, u64 ino, umode_t mode, u64 size, s64 ret), TP_ARGS(sb, ino, mode, size, ret), @@ -1220,7 +1220,7 @@ TRACE_EVENT(scoutfs_delete_inode_end, __field(__u64, ino) __field(umode_t, mode) __field(__u64, size) - __field(int, ret) + __field(__s64, ret) ), TP_fast_assign( @@ -1231,7 +1231,7 @@ TRACE_EVENT(scoutfs_delete_inode_end, __entry->ret = ret; ), - TP_printk("dev %d,%d ino %llu, mode 0x%x size %llu, ret %d", + TP_printk("dev %d,%d ino %llu, mode 0x%x size %llu, ret %lld", MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->mode, __entry->size, __entry->ret) ); From 8cea4916df643bcdcf354a987a9de709df9071d9 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Thu, 8 Oct 2026 14:27:35 -0700 Subject: [PATCH 3/6] Pending lock invalidations block lock attempts Previously the only way that pending invalidations would block lock attempts was by invalidation processing being able to set the new mode of the lock. New lock attempts ignored pending invalidations and could effectively postpone invalidation processing indefinitely by unlocking and locking fast enough. This results in visible hangs where a remote lock attempt to inspect an inode can be blocked indefinitely by active use of the inode. The basic fix of having lock attempts wait when there are pending invalidations is simple. But we have to be careful not to cause deadlocks so we only make lock attempts wait when the lock has no other users. This leaves holes where many active users can still delay invalidation but fixing that would require tracking lock ownership. Signed-off-by: Zach Brown --- kmod/src/lock.c | 47 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/kmod/src/lock.c b/kmod/src/lock.c index f20f1f69..646c1f3a 100644 --- a/kmod/src/lock.c +++ b/kmod/src/lock.c @@ -321,21 +321,29 @@ static bool lock_counts_match(int granted, unsigned int *counts) return true; } +static bool lock_counts_zero(unsigned int *counts) +{ + enum scoutfs_lock_mode mode; + + for (mode = 0; mode < SCOUTFS_LOCK_NR_MODES; mode++) { + if (counts[mode]) + return false; + } + + return true; +} + /* * An idle lock has nothing going on. It can be present in the lru and * can be freed by the final put when it has a null mode. */ static bool lock_idle(struct scoutfs_lock *lock) { - enum scoutfs_lock_mode mode; - if (lock->request_pending || lock->invalidate_pending) return false; - for (mode = 0; mode < SCOUTFS_LOCK_NR_MODES; mode++) { - if (lock->waiters[mode] || lock->users[mode]) - return false; - } + if (!lock_counts_zero(lock->waiters) || !lock_counts_zero(lock->users)) + return false; return true; } @@ -759,6 +767,26 @@ static void lock_invalidate_worker(struct work_struct *work) spin_unlock(&linfo->lock); } +/* + * Returns true if the requested mode doesn't match the next mode that + * would result after the next invalidation is processed. + * + * We have to be careful not to cause deadlocks by having a lock attempt + * wait for invalidation while the invalidation is waiting for the lock + * attempt to unlock any previous locks it might have. This can get + * gnarly when there are multiple invalidations queued and requests + * pending for elevated modes. We avoid all that and never let + * invalidations block lock attempts when there are any other users of + * the lock. + */ +static bool next_invalidate_blocks(struct scoutfs_lock *lock, u8 requested) +{ + struct inv_req *ireq = list_first_entry_or_null(&lock->inv_list, struct inv_req, head); + + return ireq && lock_counts_zero(lock->users) && + !lock_modes_match(ireq->nl.new_mode, requested); +} + /* * Add an incoming invalidation request to the end of the list on the * lock and queue it for blocking invalidation work. This is being @@ -950,7 +978,8 @@ static bool lock_wait_cond(struct super_block *sb, struct scoutfs_lock *lock, bool wake; spin_lock(&linfo->lock); - wake = linfo->shutdown || lock_modes_match(lock->mode, mode) || + wake = linfo->shutdown || + (lock_modes_match(lock->mode, mode) && !next_invalidate_blocks(lock, mode)) || !lock->request_pending; spin_unlock(&linfo->lock); @@ -1026,7 +1055,7 @@ static int lock_key_range(struct super_block *sb, enum scoutfs_lock_mode mode, i } /* the fast path where we can use the granted mode */ - if (lock_modes_match(lock->mode, mode)) { + if (lock_modes_match(lock->mode, mode) && !next_invalidate_blocks(lock, mode)) { lock_inc_count(lock->users, mode); *ret_lock = lock; ret = 0; @@ -1040,7 +1069,7 @@ static int lock_key_range(struct super_block *sb, enum scoutfs_lock_mode mode, i break; } - if (!lock->request_pending) { + if (!lock_modes_match(lock->mode, mode) && !lock->request_pending) { lock->request_pending = 1; should_send = true; } else { From bb608269819ba1bb9d651e44487b9f82c4cb82f9 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Tue, 13 Oct 2026 14:33:03 -0700 Subject: [PATCH 4/6] Remove scoutfs_lock_unmount_begin() Remove scoutfs_lock_unmount_begin() which wasn't doing anything. This is an artifact of back when the lock invalidation in lock.c used to prune the dcache directly. That can't be done after unmount gets far enough because the fs core destroys dentries without lock coverage. So this _unmount begin was flushing invalidation and pruning work before unmount carried on. These days this is accomplished by _lock_flush_invalidate() and _inode_flush_iput(). Signed-off-by: Zach Brown --- kmod/src/lock.c | 16 ---------------- kmod/src/lock.h | 1 - kmod/src/super.c | 2 +- 3 files changed, 1 insertion(+), 18 deletions(-) diff --git a/kmod/src/lock.c b/kmod/src/lock.c index 646c1f3a..c9e59b91 100644 --- a/kmod/src/lock.c +++ b/kmod/src/lock.c @@ -78,7 +78,6 @@ struct lock_info { struct super_block *sb; spinlock_t lock; bool shutdown; - bool unmounting; struct rb_root lock_tree; struct rb_root lock_range_tree; u64 nr_locks; @@ -1516,21 +1515,6 @@ static void lock_tseq_show(struct seq_file *m, struct scoutfs_tseq_entry *ent) lock->users[SCOUTFS_LOCK_WRITE_ONLY]); } -/* - * shrink_dcache_for_umount() tears down dentries with no locking. We - * need to make sure that our invalidation won't touch dentries before - * we return and the caller calls the generic vfs unmount path. - */ -void scoutfs_lock_unmount_begin(struct super_block *sb) -{ - DECLARE_LOCK_INFO(sb, linfo); - - if (linfo) { - linfo->unmounting = true; - flush_work(&linfo->inv_work); - } -} - void scoutfs_lock_flush_invalidate(struct super_block *sb) { DECLARE_LOCK_INFO(sb, linfo); diff --git a/kmod/src/lock.h b/kmod/src/lock.h index 54c8cedb..61dc8178 100644 --- a/kmod/src/lock.h +++ b/kmod/src/lock.h @@ -110,7 +110,6 @@ u64 scoutfs_lock_ino_refresh_gen(struct super_block *sb, u64 ino); void scoutfs_free_unused_locks(struct super_block *sb); int scoutfs_lock_setup(struct super_block *sb); -void scoutfs_lock_unmount_begin(struct super_block *sb); void scoutfs_lock_flush_invalidate(struct super_block *sb); void scoutfs_lock_shutdown(struct super_block *sb); void scoutfs_lock_destroy(struct super_block *sb); diff --git a/kmod/src/super.c b/kmod/src/super.c index f2e1420f..548a5a8b 100644 --- a/kmod/src/super.c +++ b/kmod/src/super.c @@ -668,7 +668,7 @@ static void scoutfs_kill_sb(struct super_block *sb) if (SCOUTFS_HAS_SBI(sb)) { scoutfs_options_stop(sb); scoutfs_inode_orphan_stop(sb); - scoutfs_lock_unmount_begin(sb); + scoutfs_lock_flush_invalidate(sb); } kill_block_super(sb); From 462c3bb7f9d6603ebba22d527e1bbd9cb90abbc9 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Tue, 13 Oct 2026 14:45:57 -0700 Subject: [PATCH 5/6] Locks return ENOLINK during forced unmount During forced unmount lock calls only return errors today if they get an immediate error from sending a lock request. If a lock request is pending then blocked lock calls can hang. They were meant to return errors after getting error responses that are generated as the networking client is disconnected, but the response handler just returns an error itself. Without the response message it doesn't know which lock key experienced the error. Either associating requests with lock pointers or passing the initial request data payload to the response handler both have lifetime implications that make me a bit nervous. This instead has lock calls specifically return errors when we're forcing an unmount. This could be torn out if we took the time to handle error responses in the lock grant requests. Signed-off-by: Zach Brown --- kmod/src/lock.c | 46 +++++++++++++++++++++++++++++++++++++++------- kmod/src/lock.h | 1 + kmod/src/super.c | 1 + 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/kmod/src/lock.c b/kmod/src/lock.c index c9e59b91..2197df96 100644 --- a/kmod/src/lock.c +++ b/kmod/src/lock.c @@ -977,7 +977,7 @@ static bool lock_wait_cond(struct super_block *sb, struct scoutfs_lock *lock, bool wake; spin_lock(&linfo->lock); - wake = linfo->shutdown || + wake = scoutfs_forcing_unmount(sb) || linfo->shutdown || (lock_modes_match(lock->mode, mode) && !next_invalidate_blocks(lock, mode)) || !lock->request_pending; spin_unlock(&linfo->lock); @@ -1048,6 +1048,16 @@ static int lock_key_range(struct super_block *sb, enum scoutfs_lock_mode mode, i lock_inc_count(lock->waiters, mode); for (;;) { + /* + * Lock grant request error response handling is + * non-functional. We directly return when forcing + * unmount rather than via errors network requests. + */ + if (scoutfs_forcing_unmount(sb)) { + ret = -ENOLINK; + break; + } + if (WARN_ON_ONCE(linfo->shutdown)) { ret = -ESHUTDOWN; break; @@ -1523,6 +1533,32 @@ void scoutfs_lock_flush_invalidate(struct super_block *sb) flush_work(&linfo->inv_work); } +static void wake_all_locks(struct lock_info *linfo) +{ + struct scoutfs_lock *lock; + struct rb_node *node; + + spin_lock(&linfo->lock); + for (node = rb_first(&linfo->lock_tree); node; node = rb_next(node)) { + lock = rb_entry(node, struct scoutfs_lock, node); + wake_up(&lock->waitq); + } + spin_unlock(&linfo->lock); +} + +/* + * Used to break waiting lock attempts that should fail during forced + * unmount but which aren't connected to error responses from the + * canceled net requests. + */ +void scoutfs_lock_kick_waiters(struct super_block *sb) +{ + DECLARE_LOCK_INFO(sb, linfo); + + if (linfo) + wake_all_locks(linfo); +} + static u64 get_held_lock_refresh_gen(struct super_block *sb, struct scoutfs_key *start) { DECLARE_LOCK_INFO(sb, linfo); @@ -1579,8 +1615,6 @@ u64 scoutfs_lock_ino_refresh_gen(struct super_block *sb, u64 ino) void scoutfs_lock_shutdown(struct super_block *sb) { DECLARE_LOCK_INFO(sb, linfo); - struct scoutfs_lock *lock; - struct rb_node *node; if (!linfo) return; @@ -1590,11 +1624,9 @@ void scoutfs_lock_shutdown(struct super_block *sb) /* cause current and future lock calls to return errors */ spin_lock(&linfo->lock); linfo->shutdown = true; - for (node = rb_first(&linfo->lock_tree); node; node = rb_next(node)) { - lock = rb_entry(node, struct scoutfs_lock, node); - wake_up(&lock->waitq); - } spin_unlock(&linfo->lock); + + wake_all_locks(linfo); } /* diff --git a/kmod/src/lock.h b/kmod/src/lock.h index 61dc8178..a20f263b 100644 --- a/kmod/src/lock.h +++ b/kmod/src/lock.h @@ -111,6 +111,7 @@ void scoutfs_free_unused_locks(struct super_block *sb); int scoutfs_lock_setup(struct super_block *sb); void scoutfs_lock_flush_invalidate(struct super_block *sb); +void scoutfs_lock_kick_waiters(struct super_block *sb); void scoutfs_lock_shutdown(struct super_block *sb); void scoutfs_lock_destroy(struct super_block *sb); diff --git a/kmod/src/super.c b/kmod/src/super.c index 548a5a8b..5e360b13 100644 --- a/kmod/src/super.c +++ b/kmod/src/super.c @@ -249,6 +249,7 @@ static void scoutfs_umount_begin(struct super_block *sb) sbi->forced_unmount = true; scoutfs_client_net_shutdown(sb); + scoutfs_lock_kick_waiters(sb); } static const struct super_operations scoutfs_super_ops = { From 081d378b50f5c075834c33db0d2e0f928165e93c Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Wed, 14 Oct 2026 14:31:52 -0700 Subject: [PATCH 6/6] Have large-fragmented-free wait for orphans We've seen deletion processing from large-fragmented-free continue on into later tests. Let's see if this makes sure that deletion finishes during this test. Signed-off-by: Zach Brown --- tests/tests/large-fragmented-free.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/tests/large-fragmented-free.sh b/tests/tests/large-fragmented-free.sh index e27d1bbe..5272f7e7 100644 --- a/tests/tests/large-fragmented-free.sh +++ b/tests/tests/large-fragmented-free.sh @@ -19,4 +19,12 @@ rm -f "$T_D0/move" echo "== cleanup" rm -f "$T_D0/alloc" +# +# If the inodes were picked up by the scanners then extent freeing by +# final deletion could continune long after this text exited. Make sure +# the scanners are idle before finish so later tests don't have +# surprising background work. +# +t_wait_for_no_orphans + t_pass