Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Apr 2023 16:31:24 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Yangtao Li <frank.li@vivo.com>
Cc:     chao@kernel.org, damien.lemoal@opensource.wdc.com,
        huyue2@coolpad.com, jefflexu@linux.alibaba.com, jth@kernel.org,
        linux-erofs@lists.ozlabs.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, naohiro.aota@wdc.com,
        rafael@kernel.org, xiang@kernel.org
Subject: Re: [PATCH 2/3] erofs: convert to use kobject_is_added()
Message-ID: <2023040602-stack-overture-d418@gregkh>
References: <2023040635-duty-overblown-7b4d@gregkh>
 <20230406120716.80980-1-frank.li@vivo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230406120716.80980-1-frank.li@vivo.com>
Precedence: bulk

On Thu, Apr 06, 2023 at 08:07:16PM +0800, Yangtao Li wrote:
> > Meta-comment, we need to come up with a "filesystem kobject type" to get
> > rid of lots of the boilerplate filesystem kobject logic as it's
> > duplicated in every filesystem in tiny different ways and lots of times
> > (like here), it's wrong.
> 
> Can we add the following structure?
> 
> struct filesystem_kobject {
>         struct kobject kobject;
>         struct completion unregister;
> };

Ah, no, I see the problem.

The filesystem authors are treating the kobject NOT as the thing that
handles the lifespan of the structure it is embedded in, but rather as
something else (i.e. a convient place to put filesystem information to
userspace.)

That isn't going to work, and as proof of that, the release callback
should be a simple call to kfree(), NOT as a completion notification
which then something else will go off and free the memory here.  That
implies that there are multiple reference counting structures happening
on the same structure, which is not ok.

Either we let the kobject code properly handle the lifespan of the
structure, OR we pull it out of the structure and just let it hang off
as a separate structure (i.e. a pointer to something else.)

As the superblock lifespan rules ALREADY control the reference counting
logic of the filesystem superblock structure, let's stick with that and
just tack-on the kobject as a separate structure entirely.

Does that make sense?  Let me do a quick pass at this for zonefs as
that's pretty simple to show you what I mean...

thanks,

greg k-h