On Tue, 11 Feb 2025, Kent Overstreet wrote: > 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... Cool - thanks. > > Nit: I would go back and stare at the patch some more, but threading got > completely fubar so I can't find anything. Doh. > :-) NeilBrown > > My patch will change it back to -ENOENT - the way you originally wrote > > it. > > > > I hope you are ok with that. > > Yes, sounds good. >
