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

James Bottomley
2025-01-19T14:57:04Z

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.

Regards,

James