Re: lsm sb_delete hook, was Re: [PATCH 4/7] vfs: Convert sb->s_inodes iteration to super_iter_inodes()
From: Jan Kara <jack@suse.cz>
Date: 2024-10-03 12:56:56
Also in:
linux-bcachefs, linux-fsdevel, linux-xfs
On Thu 03-10-24 05:39:23, Christoph Hellwig wrote:
On Thu, Oct 03, 2024 at 02:26:57PM +0200, Jan Kara wrote:quoted
On Thu 03-10-24 05:11:11, Christoph Hellwig wrote:quoted
On Thu, Oct 03, 2024 at 01:57:21PM +0200, Jan Kara wrote:quoted
Fair enough. If we go with the iterator variant I've suggested to Dave in [1], we could combine the evict_inodes(), fsnotify_unmount_inodes() and Landlocks hook_sb_delete() into a single iteration relatively easily. But I'd wait with that convertion until this series lands.I don't see how that has anything to do with iterators or not.Well, the patches would obviously conflictConflict with what?
I thought you wanted the interations to be unified in current state of code. If you meant after Dave's series, then we are in agreement.
quoted
which seems pointless if we could live with three iterations for a few years until somebody noticed :). And with current Dave's version of iterators it will not be possible to integrate evict_inodes() iteration with the other two without a layering violation. Still we could go from 3 to 2 iterations.What layering violation? Below is quick compile tested part to do the fsnotify side and get rid of the fsnotify iteration, which looks easily worth it.
...
quoted hunk ↗ jump to hunk
@@ -789,11 +789,23 @@ static bool dispose_list(struct list_head *head) */ static int evict_inode_fn(struct inode *inode, void *data) { + struct super_block *sb = inode->i_sb; struct list_head *dispose = data; + bool post_unmount = !(sb->s_flags & SB_ACTIVE); spin_lock(&inode->i_lock); - if (atomic_read(&inode->i_count) || - (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE))) { + if (atomic_read(&inode->i_count)) { + spin_unlock(&inode->i_lock); + + /* for each watch, send FS_UNMOUNT and then remove it */ + if (post_unmount && fsnotify_sb_info(sb)) { + fsnotify_inode(inode, FS_UNMOUNT); + fsnotify_inode_delete(inode); + }
This will not work because you are in unsafe iterator holding sb->s_inode_list_lock. To be able to call into fsnotify, you need to do the iget / iput dance and releasing of s_inode_list_lock which does not work when a filesystem has its own inodes iterator AFAICT... That's why I've called it a layering violation. Honza
+ return INO_ITER_DONE;
+ }
+
+ if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
spin_unlock(&inode->i_lock);
return INO_ITER_DONE;
}-- Jan Kara [off-list ref] SUSE Labs, CR