Re: [GIT PULL] gpio: updates for v5.13
From: Bartosz Golaszewski
Date: Tue May 04 2021 - 10:17:07 EST
On 5/4/21, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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