Received-SPF: pass (google.com: domain of linux-kernel+bounces-139231-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 10 Apr 2024 18:57:36 +0000
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
From: "Eric Van Hensbergen" <eric.vanhensbergen@linux.dev>
Message-ID: <e73a1e0c90ebce33c23f8f7746c23c1199f62a78@linux.dev>
TLS-Required: No
Subject: Re: [GIT PULL] fs/9p patches for 6.9 merge window
To: "Oleg Nesterov" <oleg@redhat.com>
Cc: v9fs@lists.linux.dev, linux-kernel@vger.kernel.org,
 kent.overstreet@linux.dev
In-Reply-To: <74f117635037a82dc2fb2923993cf329b6939b7e@linux.dev>
References: <ZfRkyxUf8TIgsYjA@1149290c588b>
 <20240408141436.GA17022@redhat.com>
 <74f117635037a82dc2fb2923993cf329b6939b7e@linux.dev>

April 10, 2024 at 12:20 PM, "Eric Van Hensbergen" <eric.vanhensbergen@lin=
ux.dev> wrote:
> April 8, 2024 at 9:14 AM, "Oleg Nesterov" <oleg@redhat.com> wrote:
> > Hello,
> >  the commit 724a08450f74 ("fs/9p: simplify iget to remove unnecessary=
 paths")
> >  from this PR breaks my setup.
> >=20
>=20
> Thanks for the bisect and detailed reproduction instructions. I am look=
ing at this now and it seems to be related to another problem reported by=
 Kent Overstreet where he was seeing symlink loop reports that were disru=
pting his testing environment. Once I'm able to reproduce, I'll try and g=
et a patch out later today, if not I may revert the commit to keep from d=
isrupting folks' testing environments.
>=20

Okay,=20I think I understand this one, unfortunately it doesn't appear ob=
viously related to Kent's report if what I believe is correct.  I think I=
've reproduced the problem, fundamentally, since you have two mount point=
s you are exporting together. I believe we are getting an inode number co=
llision which was being hidden by the "always create a new inode on looku=
p" inefficiency in v9fs_vfs_lookup.  You could probably verify that for m=
e by stating the /home directory and the / directory on the server side o=
f your setup.  When I created two partitions and mounted one inside the o=
ther the "home" and the "root" both had inode 2 and I got -ELOOP when try=
ing to access.

The underlying cause in the patch series is that I was trying to maintain=
 inodes versus constantly creating and deleting them -- and I simplified =
the inode lookup to be based purely on inode number (versus checking agai=
nst times, type, i_generation, etc.) -- so collisions are much more likel=
y to happen.  If qemu detects that this is a possibility it usually print=
s something:=20
qemu-system-aarch64:=20warning: 9p: Multiple devices detected in same Vir=
tFS export, which might lead to file ID collisions and severe misbehaviou=
rs on guest! You should either use a separate export for each device shar=
ed from host or use virtfs option 'multidevs=3Dremap'!

I can confirm that multidevs=3Dremap in qemu does appear to avoid the pro=
blem, but this doesn't help the fact that we have a broader regression on=
 our hand that used to work.  It'd be useful to see if mutlidevs=3Dremap =
also deals with your problem @Kent, but while I think its the same underl=
ying problem, I don't think it may be the same solution.

I think I have to give up on relying on qid.path/inode-number as unique a=
nd maintain our own client view of those -- but this will cause problems =
with the way several parts of the code currently operate where we need to=
 lookup inode by a unique identifier from the server (which is currently =
conveyed by qids).

Now that I understand the problem, I think I can work thorugh a partial r=
evert which will go back to a more complex match in vfs_lookup (which exa=
mines several other fields from the server beyond qid.path) -- but maybe =
I can do it in such a way which will still avoid keeping duplicate inode =
structs around in memory.

This didn't show up in my regressions because I always export a single fi=
le system where the inodes are always unique.

Thanks for your patience while I work through this - now that i can repro=
duce the problem, I'm fairly certain I can get a patch together this week=
 to test to see if it solves the regressions.

     -eric