On Wed, 8 Nov 2023 at 01:54, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> On Tue, Oct 31, 2023 at 05:49:19PM -0300, Wedson Almeida Filho wrote:
> > On Sun, 29 Oct 2023 at 23:29, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 23, 2023 at 09:55:08AM -0300, Wedson Almeida Filho wrote:
> > > > On Mon, 23 Oct 2023 at 02:29, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> > If you do, then you'd have to agree that we are in undefined-behaviour
> > territory. I can quote the spec if you'd like.
>
> /me shrugs
>
> I can point you at lots of code that it will break if bit operations
> are allowed to randomly change other bits in the word transiently.
Sure, in C you have chosen to rely on behaviour that the language spec
says is undefined.
In Rust, we're trying avoid it. When it's unavoidable, we're trying to
clearly mark it so that we can try to fix it later.
> All the rust code that calls iget_locked() needs to do to "be safe"
> is the rust equivalent of:
>
> spin_lock(&inode->i_lock);
> if (!(inode->i_state & I_NEW)) {
> spin_unlock(&inode->i_lock);
> return inode;
> }
> spin_unlock(&inode->i_lock);
>
> IOWs, we solve the "safety" concern by ensuring that Rust filesystem
> implementations follow the general rule of "always hold the i_lock
> when accessing inode->i_state" I originally outlined, yes?
Ah, the name of the functions iget_locked() and unlock_new_inode()
threw me off, I thought I wouldn't be able to lock inode->i_lock.
Ok, I will do this for now, I think it's better than relying on
undefined behaviour. Thanks!
Actually, looking at the implementation of iget_locked(), there's a
single place where it returns a new inode. Wouldn't it be better to
just return this piece of information (whether the inode is new or
not) to the caller? Then we would eliminate the data races in C and
the need to lock in Rust, and we would also eliminate a memory load
from inode->i_state in all callers.
> > But we want the be very deliberate about these. We don't want to
> > accidentally introduce data races (and therefore potential undefined
> > behaviour).
>
> The stop looking at the C code and all the exceptions we make for
> special case optimisations and just code to the generic rules for
> safe access to given fields. Yes, rust will then have to give up the
> optimisations we make in the C code, but there's always a price for
> safety...
I'm not trying to do clever optimisations at all. I'm trying to figure
out how to do things by looking at imperfect documentation in
filesystems/porting.rst (which, BTW, checks I_NEW without a lock) and
the functions I call. So I look at what existing filesystems do to
learn the hopefully most up to date way of doing things. If you have a
recommendation on how to do this more efficiently, I'm all ears!
Thanks,
-Wedson
