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. 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. My patch will change it back to -ENOENT - the way you originally wrote it. I hope you are ok with that. NeilBrown
