Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Feb 2022 15:38:48 -0800
From:   "Darrick J. Wong" <djwong@kernel.org>
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     NeilBrown <neilb@suse.de>, Dave Chinner <david@fromorbit.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Daire Byrne <daire@dneg.com>,
        Andreas Dilger <adilger.kernel@dilger.ca>
Subject: Re: [PATCH/RFC] VFS: support parallel updates in the one directory.
Message-ID: <20220224233848.GC8269@magnolia>
References: <164568221518.25116.18139840533197037520@noble.neil.brown.name>
 <893053D7-E5DD-43DB-941A-05C10FF5F396@dilger.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <893053D7-E5DD-43DB-941A-05C10FF5F396@dilger.ca>
Precedence: bulk

On Thu, Feb 24, 2022 at 09:31:28AM -0700, Andreas Dilger wrote:
> On Feb 23, 2022, at 22:57, NeilBrown <neilb@suse.de> wrote:
> > 
> > 
> > I added this:
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -87,6 +87,7 @@ xfs_inode_alloc(
> >    /* VFS doesn't initialise i_mode or i_state! */
> >    VFS_I(ip)->i_mode = 0;
> >    VFS_I(ip)->i_state = 0;
> > +    VFS_I(ip)->i_flags |= S_PAR_UPDATE;
> >    mapping_set_large_folios(VFS_I(ip)->i_mapping);
> > 
> >    XFS_STATS_INC(mp, vn_active);
> > 
> > and ran my highly sophisticated test in an XFS directory:
> > 
> > for i in {1..70}; do ( for j in {1000..8000}; do touch $j; rm -f $j ; done ) & done

I think you want something faster here, like ln to hardlink an existing
file into the directory.

> > This doesn't crash - which is a good sign.
> > While that was going I tried
> > while : ; do ls -l ; done
> > 
> > it sometimes reports garbage for the stat info:
> > 
> > total 0
> > -????????? ? ?    ?    ?            ? 1749
> > -????????? ? ?    ?    ?            ? 1764
> > -????????? ? ?    ?    ?            ? 1765
> > -rw-r--r-- 1 root root 0 Feb 24 16:47 1768
> > -rw-r--r-- 1 root root 0 Feb 24 16:47 1770
> > -rw-r--r-- 1 root root 0 Feb 24 16:47 1772
> > ....
> > 
> > I *think* that is bad - probably the "garbage" that you referred to?
> > 
> > Obviously I gets lots of 
> > ls: cannot access '1764': No such file or directory
> > ls: cannot access '1749': No such file or directory
> > ls: cannot access '1780': No such file or directory
> > ls: cannot access '1765': No such file or directory
> > 
> > but that is normal and expected when you are creating and deleting
> > files during the ls.
> 
> The "ls -l" output with "???" is exactly the case where the filename is
> in readdir() but stat() on a file fails due to an unavoidable userspace 
> race between the two syscalls and the concurrent unlink(). This is
> probably visible even without the concurrent dirops patch. 
> 
> The list of affected filenames even correlates with the reported errors:
> 1764, 1765, 1769
> 
> It looks like everything is working as expected. 

Here, yes.

A problem that I saw a week or two ago with online fsck is that an evil
thread repeatedly link()ing and unlink()ing a file into an otherwise
empty directory while racing a thread calling readdir() in a loop will
eventually trigger a corruption report on the directory namecheck
because the loop in xfs_dir2_sf_getdents that uses sfp->count as a loop
counter will race with the unlink decrementing sfp->count and run off
the end of the inline directory data buffer.

https://elixir.bootlin.com/linux/latest/source/fs/xfs/xfs_dir2_readdir.c#L121

The solution in that case was a forgotten acquisition of the directory
IOLOCK, but I don't see why the same principle wouldn't apply here.
It's probably not so hard to fix it (rewrite readdir to take the ILOCK
once, format the dirents to a buffer until it's full, save cursor, drop
ILOCK, copy buffer to userspace) but it's not as easy as setting
PAR_UPDATE.

(I am also not a fan of "PAR_UPDATE", since 'par' is already an English
word that doesn't mean 'parallel'.)

--D

> 
> Cheers, Andreas
>