Received-SPF: pass (google.com: domain of linux-kernel+bounces-20131-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 9 Jan 2024 06:37:09 +0900
From: asmadeus@codewreck.org
To: Christian Schoenebeck <linux_oss@crudebyte.com>
Cc: Eric Van Hensbergen <ericvh@kernel.org>, linux-kernel@vger.kernel.org,
	v9fs@lists.linux.dev, rminnich@gmail.com, lucho@ionkov.net
Subject: Re: [PATCH] fs/9p: fix inode nlink accounting
Message-ID: <ZZxrBbR2YHz0E5v0@codewreck.org>
References: <20240107-fix-nlink-handling-v1-1-8b1f65ebc9b2@kernel.org>
 <8004884.rDQMAZhJ5Z@silver>
 <CAFkjPTn0HFnnZk8rt7m+mfLyPofMxZC6EzaoFChoyRVdVnwM6Q@mail.gmail.com>
 <7785659.j189Hiylts@silver>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <7785659.j189Hiylts@silver>

Christian Schoenebeck wrote on Mon, Jan 08, 2024 at 03:55:53PM +0100:
> > I'm good with adding local wrapper functions,

(Agreed having a local wrapper that locks + use these is better than this
current patch -- v2 looks much better, thanks!)

> > I imagine these aren't
> > used in the kernel because for regular file-systems maybe you want the
> > warning that your inode link accounting is wrong.
> > I suppose we could be naughty and not use the kernel functions (which
> > themselves are basically wrappers).
> 
> Well, one half of that code is actually using atomic operations to increment/
> decrement the private counter. Which means to me those kernel functions were
> intended to be called from a concurrent context. So I don't get why the other
> variable is not atomic there. They should be I think.

I think the key difference is inode level vs superblock level -- the
inode is local and holding a lock can be faster if manipulations are
grouped together (x atomic operations are usually slower than a spinlock
and x normal operations), while the sb potentially has contentions and
would be more likely to use atomic...

> I would probably try and send a patch for changing those kernel functions and
> see if people are fine with that. But up to you.

With that said I just checked ext4 and it looks just as racy as we do in
particular the rmdir/unlink case doesn't seem to take any lock, so it's
definitely worth raising the subject on fsdevel!
I'll see how work is busy and ask later today if time allows


-- 
Dominique Martinet | Asmadeus