Messages in this thread | | | From | Bartosz Golaszewski <> | Date | Tue, 4 May 2021 16:17:02 +0200 | Subject | Re: [GIT PULL] gpio: updates for v5.13 |
| |
On 5/4/21, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Mon, May 03, 2021 at 06:28:38PM +0000, Al Viro wrote: > >> > So Al, do you see anything horrendous in how that configfs thing uses >> > a rename to do kind of an "atomic swap" of configfs state? >> >> Give me a few hours; configfs is playing silly buggers with a lot of >> structures when creating/tearing down subtrees, and I'd actually >> expect more trouble with configfs data structures than with VFS ones. >> >> I'll take a look. >
Hi Al and thanks for the comments!
> FWIW, one obviously bogus thing is this: > > + spin_lock(&configfs_dirent_lock); > + new_dentry->d_fsdata = sd; > + list_move(&sd->s_sibling, &new_parent_sd->s_children); > + item->ci_parent = new_parent_item; > + d_move(old_dentry, new_dentry); > + spin_unlock(&configfs_dirent_lock); > on successful ->rename(). sd here comes from > + sd = old_dentry->d_fsdata; > > Now, take a look at configfs_d_iput(). ->d_fsdata contributes > to refcount of sd, and I don't see anything here that would grab the > reference. > > Incidentally, if your code critically depends upon some field > being first in such-and-such structure, you should either get rid of > the dependency or at least bother to document that. > That > + /* > + * Free memory allocated for the pending and live > directories > + * of committable groups. > + */ > + if (sd->s_type & (CONFIGFS_GROUP_PENDING | > CONFIGFS_GROUP_LIVE)) > + kfree(sd->s_element); > + > is asking for trouble down the road. >
I'm not sure if this is a hard NAK for these changes or if you consider this something that can be ironed out post v5.13-rc1?
> I dislike (for the lack of adequate printable terms) the way configfs > deals with subtree creation and, especially, removal. It's kept attached > to dentry tree (all the way to the root) as we build it and, in case we > fail halfway through, as we are trying to take it apart. > > There is convoluted code trying to prevent breakage in such cases, > but it's complex, brittle and I don't remember how critical the lack of > renames had been in its analysis. I can try to redo that, but that would > take some time - IIRC, the last time I did it, it took several days > of work (including arseloads of grepping through configfs users and > doing RTFS in those) > > IMO we should attach the subtree we'd built only when it's > fully set up. I can dig out the notes (from 2 years ago) on how to massage > the damn thing in that direction, but again, it'll take a day or two > to verify that analysis still applies. OTOH, that would simplify the code > considerably, so the next time we want to change something it wouldn't > be so unpleasant. >
This seems to address fundamental issues with configfs. I probably don't have enough deep understanding of the VFS to even try to take on this. My question again is: should this block the committable items from getting merged or is this a plan for future improvement?
Can we proceed with merging it to see if it causes any regressions later in the release cycle?
IMO this isn't a case where we could corrupt someone's files if we make a mistake but I also acknowledge that I'm biased because I'm the one who wants this functionality to improve our user-space tests.
Best Regards Bartosz Golaszewski
| |