On Thu 03-10-24 05:39:23, Christoph Hellwig wrote:
> On Thu, Oct 03, 2024 at 02:26:57PM +0200, Jan Kara wrote:
> > On Thu 03-10-24 05:11:11, Christoph Hellwig wrote:
> > > On Thu, Oct 03, 2024 at 01:57:21PM +0200, Jan Kara wrote:
> > > > 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 conflict
>
> Conflict 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.
> > 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.
...
> @@ -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 <jack@xxxxxxxx>
SUSE Labs, CR
