On Tue, 2025-10-28 at 22:34 +0100, Ard Biesheuvel wrote: > On Tue, 28 Oct 2025 at 22:08, Al Viro <[email protected]> > wrote: > > > > On Tue, Oct 28, 2025 at 05:45:40PM +0000, Al Viro wrote: > > > > > FWIW, having a special path for "we are in foofs_fill_super(), > > > fuck the locking - nobody's going to access it anyway" is not a > > > great idea, simply because the helpers tend to get reused on > > > codepaths where we can't cut corners that way. > > > > BTW, looking through efivarfs codebase now... *both* > > callers of efivarfs_create_dentry() end up doing dcache lookups, > > with variously convoluted call chains. Look: > > efivarfs_check_missing() has an explicit try_lookup_noperm() before > > the call of efivarfs_create_dentry(). efivarfs_callback() doesn't, > > but it's called via > > efivar_init(efivarfs_callback, sb, true) > > and with the last argument being true efivar_init() will precede > > the call of the callback with efivarfs_variable_is_present(). > > Guess what does that thing (never used anywhere else) do? Right, > > the call of try_lookup_noperm(). > > > > Why do we bother with that? What's wrong with having > > efivarfs_create_dentry() returning -EEXIST in case of dentry > > already being there and turning the chunk in efivar_init() into > > err = func(variable_name, vendor_guid, > > variable_name_size, data); > > if (err == -EEXIST) { > > if (duplicate_check) > > > > dup_variable_bug(variable_name, > > > > &vendor_guid, > > > > variable_name_size); > > else > > err = 0; > > } > > if (err) > > status = EFI_NOT_FOUND; > > Note that both possible callbacks become almost identical and I > > wouldn't be surprised if that "almost" is actually "completely"... > > <checks> yep. > > > > I'll let James respond to the specifics of your suggestion, but I'll > just note that this code has a rather convoluted history, as we used > to have two separate pseudo-filesystem drivers, up until a few years > ago: the sysfs based 'efivars' and this efivarfs driver. Given that > modifications in one needed to be visible in the other, they shared a > linked list that shadowed the state of the underlying variable store. > 'efivars' was removed years ago, but it was only recently that James > replaced the linked list in this driver with the dentry cache as the > shadow mechanism.
I think this all looks OK. The reason for the convolution is that simple_start/done_creating() didn't exist when I did the conversion ... although if they had, I'm not sure I'd have thought of reworking efivarfs_create_dentry to use them. I tried to update some redundant bits, but it wasn't the focus of what I was trying to fix. So I think the cleanup works and looks nice. > > Relying on the -EEXIST return value to detect duplicates, and > combining the two callbacks seem like neat optimizations to me, so > > Acked-by: Ard Biesheuvel <[email protected]> > > but I have to confess I am slightly out of my depth when it comes to > VFS stuff. Yes, ack too. Regards, James
