Message-ID: <20250516063447.GA14632@lst.de>
Date: Fri, 16 May 2025 08:34:47 +0200
From: Christoph Hellwig <hch@....de>
To: Dave Chinner <david@...morbit.com>
Cc: Christoph Hellwig <hch@....de>, Carlos Maiolino <cem@...nel.org>,
	linux-xfs@...r.kernel.org, cen zhang <zzzccc427@...il.com>,
	lkmm@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: mark the i_delayed_blks access in
 xfs_file_release as racy

On Thu, May 15, 2025 at 09:21:01AM +1000, Dave Chinner wrote:
> > I'd like to understand that one a bit more.  It might be because the
> > validator doesn't understand a semaphore used as lock is a lock, but
> > I'll follow up there.
> 
> Even so, it's not a race because at that point in time the object
> cannot be seen by anything other than the process that is
> initialising it.

In the current tree after the folio/vmalloc series b_addr is only
assigned during buffer allocation.  But I suspect they tested before that
where b_addr can be set at runtime.  Either way it always happens under
b_sema because that is initialized to locked just after allocating
the memory for the buffer.

> I'm wary of this, because at this point I suspect that there aren't
> a lot of people with sufficient time and knowledge to adequately
> address these issues.

I'm more than happy to address these, because proper documentation
of concurrency helps fixing a huge number of bugs, and also really
helps documenting the code.  I hate having to spend hours trying to
figure out why something can be safely used lockless or not.

> We should have learnt this lesson from lockdep - false positive
> whack-a-mole shut up individual reports but introduced technical
> debt that had to be addressed later because whack-a-mole didn't
> address the underlying issues.

I'm not sure who "we" is, but I've always pushed back to hacks just
to shut up lockdep.  And at the same time I'm immensively grateful
for having lockdep and can't think of working without it anymore.

> We need functions like xfs_vn_getattr(), the allocation AG selection
> alogrithms and object initialisation functions to be marked as
> inherently racy, because they intentionally don't hold locks for any
> of the accesses they make. kcsan provides:

Functions never are racy, specific data access are.  So a function wide
assignment is just the dumbest thing ever, this already badly failed
for things like early Java object-level synchronization.

> 
> For variables like ip->i_delayed_blks, where we intentionally
> serialise modifications but do not serialise readers, we have:
> 
> -	uint64_t                i_delayed_blks; /* count of delay alloc blks */
> +	uint64_t __data_racy    i_delayed_blks; /* count of delay alloc blks */
> 
> This means all accesses to the variable are considered to be racy
> and kcsan will ignore it and not report it. We can do the same for
> lip->li_lsn and other variables.

But not all access are racy.  We rely on proper synchronized accesses
for accounting.  Now for something that has a lot of unsynchronized
access, adding a wrapper for them might be fine, but for i_delayed_blks
I don't think we actually have enough for them to bother.

> IOWs, we do not need to spew data_race() wrappers or random
> READ_ONCE/WRITE_ONCE macros all over the code to silence known false
> positives.  If we mark the variables and functions we know have racy
> accesses, these one-line changes will avoid -all- false positives on
> those variables/from those functions.

And also drop a lot of the actually useful checks. That's exaxctly
the kind of hack you rant about above.

Powered by blists - more mailing lists