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
Date: Tue, 9 Jan 2024 06:37:09 +0900 [thread overview]
Message-ID: <ZZxrBbR2YHz0E5v0@codewreck.org> (raw)
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
prev parent reply other threads:[~2024-01-08 21:37 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-07 19:07 [PATCH] fs/9p: fix inode nlink accounting Eric Van Hensbergen
2024-01-08 11:19 ` asmadeus
2024-01-08 12:08 ` Christian Schoenebeck
2024-01-08 14:12 ` Eric Van Hensbergen
2024-01-08 14:55 ` Christian Schoenebeck
2024-01-08 21:37 ` asmadeus [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZZxrBbR2YHz0E5v0@codewreck.org \
--to=asmadeus@codewreck.org \
--cc=ericvh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux_oss@crudebyte.com \
--cc=lucho@ionkov.net \
--cc=rminnich@gmail.com \
--cc=v9fs@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).