Re: [PATCH v2 4/6] efivarfs: move freeing of variable entry into evict_inode

Ard Biesheuvel
2025-01-19T16:31:30Z

On Sun, 19 Jan 2025 at 15:57, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Sun, 2025-01-19 at 15:50 +0100, Ard Biesheuvel wrote:
> > On Thu, 16 Jan 2025 at 23:13, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > >
> > > On Thu, 2025-01-16 at 14:05 -0500, James Bottomley wrote:
> > > > On Thu, 2025-01-16 at 18:36 +0000, Al Viro wrote:
> > > > > On Mon, Jan 06, 2025 at 06:35:23PM -0800, James Bottomley
> > > > > wrote:
> > > > > > Make the inodes the default management vehicle for struct
> > > > > > efivar_entry, so they are now all freed automatically if the
> > > > > > file is removed and on unmount in kill_litter_super().
> > > > > > Remove the now superfluous iterator to free the entries after
> > > > > > kill_litter_super().
> > > > > >
> > > > > > Also fixes a bug where some entry freeing was missing causing
> > > > > > efivarfs to leak memory.
> > > > >
> > > > > Umm... I'd rather coallocate struct inode and struct
> > > > > efivar_entry; that way once you get rid of the list you don't
> > > > > need - evict_inode() anymore.
> > > > >
> > > > > It's pretty easy - see e.g.
> > > > > https://lore.kernel.org/all/20250112080705.141166-1-viro@zeniv.linux.org.uk/
> > > > > for recent example of such conversion.
> > > >
> > > > OK, I can do that. Although I think since the number of
> > > > variables is usually around 150, it would probably be overkill to
> > > > give it its own inode cache allocator.
> > >
> > > OK, this is what I've got. As you can see from the diffstat it's
> > > about 10 lines more than the previous; mostly because of the new
> > > allocation routine, the fact that the root inode has to be special
> > > cased for the list and the guid has to be parsed in efivarfs_create
> > > before we have the inode.
> > >
> >
> > That looks straight-forward enough.
> >
> > Can you send this as a proper patch? Or would you prefer me to squash
> > this into the one that is already queued up?
>
> Sure; I've got a slightly different version because after talking with
> Al he thinks it's OK still to put a pointer to the efivar_entry in
> i_private, which means less disruption. But there is enough disruption
> that the whole of the series needs redoing to avoid conflicts.
>
> > I'm fine with either, but note that I'd still like to target v6.14
> > with this.
>
> Great, but I'm afraid the fix for the zero size problem also could do
> with being a precursor which is making the timing pretty tight.
>

OK, I'll queue up your v3 about I won't send it out with the initial
pull request, so we can make up our minds later.

I take it the setattr series is intended for merging straight away?