Re: [PATCH v2 1/2] vfs: add rcu-based find_inode variants for iget ops

[Mateusz Guzik <mjguzik@gmail.com>]

On Tue, Jun 11, 2024 at 6:59 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +EXPORT_SYMBOL(iget5_locked_rcu);
>
> EXPORT_SYMBOL_GPL for rcu APIs.
>

noted for v3, thanks

> > +static void __wait_on_freeing_inode(struct inode *inode, bool locked)
> >  {
> >       wait_queue_head_t *wq;
> >       DEFINE_WAIT_BIT(wait, &inode->i_state, __I_NEW);
> >       wq = bit_waitqueue(&inode->i_state, __I_NEW);
> >       prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
> >       spin_unlock(&inode->i_lock);
> > -     spin_unlock(&inode_hash_lock);
> > +     rcu_read_unlock();
> > +     if (locked)
> > +             spin_unlock(&inode_hash_lock);
>
> The conditional locking here is goign to make sparse rather unhappy.
> Please try to find a way to at least annotate it, or maybe find
> another way around like, like leaving the schedule in finish_wait
> in the callers.
>

So I tried out sparse on my patch vs fs-next and found it emits the
same warnings.

fs/inode.c:846:17: warning: context imbalance in 'inode_lru_isolate' -
unexpected unlock
fs/inode.c:901:9: warning: context imbalance in 'find_inode' -
different lock contexts for basic block
fs/inode.c:932:9: warning: context imbalance in 'find_inode_fast' -
different lock contexts for basic block
fs/inode.c:1621:5: warning: context imbalance in 'insert_inode_locked'
- wrong count at exit
fs/inode.c:1739:20: warning: context imbalance in 'iput_final' -
unexpected unlock
fs/inode.c:1753:6: warning: context imbalance in 'iput' - wrong count at exit
fs/inode.c:2238:13: warning: context imbalance in
'__wait_on_freeing_inode' - unexpected unlock

The patch does not make things *worse*, so I don't think messing with
the code is warranted here.

> > +extern struct inode *ilookup5_nowait_rcu(struct super_block *sb,
> > +             unsigned long hashval, int (*test)(struct inode *, void *),
> > +             void *data);
>
> No need for the extern here (or down below).
>

I agree, but this is me just copying and modifying an existing line.

include/linux/fs.h is chock full of extern-prefixed func declarations,
on top of that some name the arguments while the rest does not.

Someone(tm) should definitely clean it up, but I'm not interested in
bikeshedding about it.

-- 
Mateusz Guzik <mjguzik gmail.com>