Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 26 Jun 2023 15:25:11 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Benjamin Coddington <bcodding@redhat.com>
Cc:     linux-nfs@vger.kernel.org, Trond Myklebust <trondmy@gmail.com>
Subject: Re: [PATCH v4 05/11] NFS: add superblock sysfs entries
Message-ID: <20230626222511.GA508950@dev-arch.thelio-3990X>
References: <cover.1686851158.git.bcodding@redhat.com>
 <095dda5e682c8367335b9fa448f2834b9435ee69.1686851158.git.bcodding@redhat.com>
 <20230626211223.GA3771155@dev-arch.thelio-3990X>
 <BB4C7A5D-BC97-4422-B5DE-D9267EB29882@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BB4C7A5D-BC97-4422-B5DE-D9267EB29882@redhat.com>
Precedence: bulk

On Mon, Jun 26, 2023 at 05:47:21PM -0400, Benjamin Coddington wrote:
> On 26 Jun 2023, at 17:12, Nathan Chancellor wrote:
> 
> > Hi Benjamin,
> >
> > On Thu, Jun 15, 2023 at 02:07:26PM -0400, Benjamin Coddington wrote:
> >> Create a sysfs directory for each mount that corresponds to the mount's
> >> nfs_server struct.  As the mount is being constructed, use the name
> >> "server-n", but rename it to the "MAJOR:MINOR" of the mount after assigning
> >> a device_id. The rename approach allows us to populate the mount's directory
> >> with links to the various rpc_client objects during the mount's
> >> construction.  The naming convention (MAJOR:MINOR) can be used to reference
> >> a particular NFS mount's sysfs tree.
> >>
> >> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> >
> > I am not sure if this has been reported or fixed already, so I apologize
> > if this is a duplicate. After this change landed in -next as commit
> > 1c7251187dc0 ("NFS: add superblock sysfs entries"), I see the following
> > splat when accessing a NFS server:
> 
> Hi Nathan, oh yes - I see there are a few paths through nfs4_init_server()
> where we can exit early due to an error or duplicate client, in which case
> nfs_free_server() tries to clean up the server kobject before it has been
> initialized.
> 
> I think we can simply do:
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 4967ac800b14..4046072663f2 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -1013,8 +1013,10 @@ void nfs_free_server(struct nfs_server *server)
> 
>         nfs_put_client(server->nfs_client);
> 
> -       nfs_sysfs_remove_server(server);
> -       kobject_put(&server->kobj);
> +       if (server->kobj.state_initialized) {
> +               nfs_sysfs_remove_server(server);
> +               kobject_put(&server->kobj);
> +       }
>         ida_free(&s_sysfs_ids, server->s_sysfs_id);
> 
>         ida_destroy(&server->lockowner_id);
> 
> Are you able to test that?  If not, totally fine - I think I should be able
> to reproduce the problem and send a patch.

Yes, that appears to work for me! Feel free to add

Tested-by: Nathan Chancellor <nathan@kernel.org>

to that diff if you send it along formally.

Cheers,
Nathan