Received-SPF: pass (google.com: domain of linux-nfs+bounces-3823-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 14 Jun 2024 13:33:08 +0200
From: Jan Kara <jack@suse.cz>
To: Jeff Layton <jlayton@kernel.org>
Cc: Jan Kara <jack@suse.cz>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	linux-nfs@vger.kernel.org, Neil Brown <neilb@suse.de>
Subject: Re: [PATCH 2/3] nfs: Properly initialize server->writeback
Message-ID: <20240614113308.sqgrnzdczbw2za5a@quack3>
References: <20240612153022.25454-1-jack@suse.cz>
 <20240613082821.849-2-jack@suse.cz>
 <4ccfb492bd6af24f8bdfd085d369c7c94c1865d1.camel@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4ccfb492bd6af24f8bdfd085d369c7c94c1865d1.camel@kernel.org>

On Thu 13-06-24 15:56:36, Jeff Layton wrote:
> On Thu, 2024-06-13 at 10:28 +0200, Jan Kara wrote:
> > Atomic types should better be initialized with atomic_long_set()
> > instead
> > of relying on zeroing done by kzalloc(). Clean this up.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > ?fs/nfs/client.c | 2 ++
> > ?1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > index de77848ae654..3b252dceebf5 100644
> > --- a/fs/nfs/client.c
> > +++ b/fs/nfs/client.c
> > @@ -994,6 +994,8 @@ struct nfs_server *nfs_alloc_server(void)
> > ?
> > ?	server->change_attr_type = NFS4_CHANGE_TYPE_IS_UNDEFINED;
> > ?
> > +	atomic_long_set(&server->writeback, 0);
> > +
> > ?	ida_init(&server->openowner_id);
> > ?	ida_init(&server->lockowner_id);
> > ?	pnfs_init_server(server);
> 
> I'm guilty of doing this, well, all over the place. Is there any
> plausible scenario where another task could see this value set to non-
> zero after kzalloc()? One would hope not...

No, it is not a practical problem these days. It is more a theoretical
correctness thing that atomic_t should be initialized through atomic_set()
and not memset() because in theory you don't know how some weird
architecture decides to implement it.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR