Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 10 Feb 2021 20:38:40 +0800
From:   Gao Xiang <hsiangkao@redhat.com>
To:     Chao Yu <chao@kernel.org>
Cc:     Gao Xiang <hsiangkao@aol.com>, linux-erofs@lists.ozlabs.org,
        LKML <linux-kernel@vger.kernel.org>, stable@vger.kernel.org
Subject: Re: [PATCH] erofs: initialized fields can only be observed after bit
 is set
Message-ID: <20210210123840.GA1173803@xiangao.remote.csb>
References: <20210209130618.15838-1-hsiangkao.ref@aol.com>
 <20210209130618.15838-1-hsiangkao@aol.com>
 <ac5abccb-70ad-441b-a5b0-b8808ff37c00@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ac5abccb-70ad-441b-a5b0-b8808ff37c00@kernel.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hi Chao,

On Wed, Feb 10, 2021 at 08:09:22PM +0800, Chao Yu wrote:
> Hi Xiang,
> 
> On 2021/2/9 21:06, Gao Xiang via Linux-erofs wrote:
> > From: Gao Xiang <hsiangkao@redhat.com>
> > 
> > Currently, although set_bit() & test_bit() pairs are used as a fast-
> > path for initialized configurations. However, these atomic ops are
> > actually relaxed forms. Instead, load-acquire & store-release form is
> > needed to make sure uninitialized fields won't be observed in advance
> > here (yet no such corresponding bitops so use full barriers instead.)
> > 
> > Fixes: 62dc45979f3f ("staging: erofs: fix race of initializing xattrs of a inode at the same time")
> > Fixes: 152a333a5895 ("staging: erofs: add compacted compression indexes support")
> > Cc: <stable@vger.kernel.org> # 5.3+
> > Reported-by: Huang Jianan <huangjianan@oppo.com>
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >   fs/erofs/xattr.c | 10 +++++++++-
> >   fs/erofs/zmap.c  | 10 +++++++++-
> >   2 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> > index 5bde77d70852..47314a26767a 100644
> > --- a/fs/erofs/xattr.c
> > +++ b/fs/erofs/xattr.c
> > @@ -48,8 +48,14 @@ static int init_inode_xattrs(struct inode *inode)
> >   	int ret = 0;
> >   	/* the most case is that xattrs of this inode are initialized. */
> > -	if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags))
> > +	if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags)) {
> > +		/*
> > +		 * paired with smp_mb() at the end of the function to ensure
> > +		 * fields will only be observed after the bit is set.
> > +		 */
> > +		smp_mb();
> 
> I can understand below usage, since w/o smp_mb(), xattr initialization
> could be done after set_bit(EROFS_I_EA_INITED_BIT), then other apps could
> see out-of-update xattr info after that bit check.
> 
> So what out-of-order execution do we need to avoid by adding above barrier?
> 

These is one-shot lazy initialization to delay read/parse xattr/compress
indexes to the first read since many workloads don't need such information
at all.

Without such memory barrier pairs, if two (or more) initializations runs
nearly simultaneously, the paralleled process could observe uninitialized
values (zeroed values). That is OPPO colleagues found on their products. 

Yeah, this could be somewhat kind of out-of-order, yet more specifically
called memory reordering. Xattr/compress indexes initialization could be
lazily observed by the CPU after it observed that EROFS_I_EA_INITED_BIT/
EROFS_I_Z_INITED_BIT is set. So we need memory barrier pairs to guarantee
such data ordering.

Thanks,
Gao Xiang

> Thanks,
> 
> > +	/* paired with smp_mb() at the beginning of the function. */
> > +	smp_mb();
> >   	set_bit(EROFS_I_EA_INITED_BIT, &vi->flags);
>