Received-SPF: pass (google.com: domain of linux-kernel+bounces-209265-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240610195828.474370-1-mjguzik@gmail.com> <20240610195828.474370-2-mjguzik@gmail.com>
 <ZmfZukP3a2atzQma@infradead.org>
In-Reply-To: <ZmfZukP3a2atzQma@infradead.org>
From: Mateusz Guzik <mjguzik@gmail.com>
Date: Tue, 11 Jun 2024 07:17:41 +0200
Message-ID: <CAGudoHE12-7c0kmVpKz8HyBeHt8jX8hOQ7zQxZNJ0Re7FF8r6g@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] vfs: add rcu-based find_inode variants for iget ops
To: Christoph Hellwig <hch@infradead.org>
Cc: brauner@kernel.org, viro@zeniv.linux.org.uk, jack@suse.cz, 
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, 
	linux-btrfs@vger.kernel.org, josef@toxicpanda.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jun 11, 2024 at 6:59=E2=80=AFAM Christoph Hellwig <hch@infradead.or=
g> 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 =3D 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 ex=
it
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.

--=20
Mateusz Guzik <mjguzik gmail.com>