On Tue, Jun 11, 2024 at 6:59 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> 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>
