On Mon, Feb 10, 2025 at 12:20:15PM +1100, NeilBrown wrote: > On Sat, 08 Feb 2025, Kent Overstreet wrote: > > On Fri, Feb 07, 2025 at 06:30:00PM +1100, NeilBrown wrote: > > > On Fri, 07 Feb 2025, Kent Overstreet wrote: > > > > On Fri, Feb 07, 2025 at 05:34:23PM +1100, NeilBrown wrote: > > > > > On Fri, 07 Feb 2025, Kent Overstreet wrote: > > > > > > On Fri, Feb 07, 2025 at 03:53:52PM +1100, NeilBrown wrote: > > > > > > > Do you think there could be a problem with changing the error returned > > > > > > > in this circumstance? i.e. if you try to destroy a subvolume with a > > > > > > > non-existant name on a different filesystem could getting -ENOENT > > > > > > > instead of -EXDEV be noticed? > > > > > > > > > > > > -EXDEV is the standard error code for "we're crossing a filesystem > > > > > > boundary and we can't or aren't supposed to be", so no, let's not change > > > > > > that. > > > > > > > > > > > > > > > > OK. As bcachefs is the only user of user_path_locked_at() it shouldn't > > > > > be too hard. > > > > > > > > Hang on, why does that require keeping user_path_locked_at()? Just > > > > compare i_sb... > > > > > > > > > > I changed user_path_locked_at() to not return a dentry at all when the > > > full path couldn't be found. If there is no dentry, then there is no > > > ->d_sb. > > > (if there was an ->i_sb, there would be an inode and this all wouldn't > > > be an issue). > > > > > > To recap: the difference happens if the path DOESN'T exist but the > > > parent DOES exist on a DIFFERENT filesystem. It is very much a corner > > > case and the error code shouldn't matter. But I had to ask... > > > > Ahh... > > > > Well, if I've scanned the series correctly (sorry, we're on different > > timezones and I haven't had much caffeine yet) I hope you don't have to > > keep that function just for bcachefs - but I do think the error code is > > important. > > > > Userspace getting -ENOENT and reporting -ENOENT to the user will > > inevitably lead to head banging frustration by someone, somewhere, when > > they're trying to delete something and the system is tell them it > > doesn't exist when they can see it very much does exist, right there :) > > the more precise error code is a very helpful cue... > > > > ??? > You will only get -ENOENT if there is no ent. There is no question of a > confusing error message. > If you ask for a non-exist name on the correct filesystem, you get -ENOENT > If you ask for an existing name of the wrong filesystem, you get -EXDEV > That all works as expected and always has. > > But what if you ask for a non-existing name in a directory on the > wrong filesystem? > The code you originally wrote in 42d237320e9817a9 would return > -ENOENT because that it what user_path_at() would return. Ahh - ok, I think I see where I misread before > But using user_path_at() is "wrong" because it doesn't lock the directory > so ->d_parent is not guaranteed to be stable. > Al fixed that in bbe6a7c899e7f265c using user_path_locked_at(), but > that doesn't check for a negative dentry so Al added a check to return > -ENOENT, but that was added *after* the test that returns -EXDEV. > > So now if you call subvolume_destroy on a non-existing name in a > directory on the wrong filesystem, you get -EXDEV. I think that is > a bit weird but not a lot weird. Yeah, we don't need to preserve that. As long as calling it on a name that _does_ exist on a different filesystem returns -EXDEV, that's all I care about. So assuming that's the case you can go ahead and add my acked-by... Nit: I would go back and stare at the patch some more, but threading got completely fubar so I can't find anything. Doh. > My patch will change it back to -ENOENT - the way you originally wrote > it. > > I hope you are ok with that. Yes, sounds good.
