From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vivek Goyal Subject: Re: [PATCH 11/11] ovl: Put barriers to order oi->__upperdentry and OVL_METACOPY update Date: Thu, 19 Oct 2017 11:22:21 -0400 Message-ID: <20171019152221.GC2090@redhat.com> References: <1508274358-17456-1-git-send-email-vgoyal@redhat.com> <1508274358-17456-12-git-send-email-vgoyal@redhat.com> <20171019130030.GA2090@redhat.com> <20171019145824.GB2090@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mx1.redhat.com ([209.132.183.28]:36764 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752860AbdJSPWW (ORCPT ); Thu, 19 Oct 2017 11:22:22 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-unionfs-owner@vger.kernel.org List-Id: linux-unionfs@vger.kernel.org To: Amir Goldstein Cc: overlayfs , Miklos Szeredi On Thu, Oct 19, 2017 at 06:08:32PM +0300, Amir Goldstein wrote: > On Thu, Oct 19, 2017 at 5:58 PM, Vivek Goyal wrote: > > On Thu, Oct 19, 2017 at 04:21:46PM +0300, Amir Goldstein wrote: > ... > >> > >> Process 2 will get lower dentry on open for read at 8AM > >> Process 1 will copy up file at 9AM (on CPU1) > >> Process 2 will open same file for read at 9AM (on CPU2) > >> Does it matter if process 2 gets lower or upper dentry? No. > >> It only matter that IF process 2 gets an upper dentry, that > >> this dentry is consistent, so it only matters that IF __upperdentry > >> is visible to CPU2 AND OVL_UPPER_DATA flag is visible to > >> CPU2 then dentry and its inode are consistent. > > > > That's a good point. So if OVL_UPPER_DATA update is not visible on CPU2 > > yet, then CPU1 will use lower dentry. And this is equivalent to as if file > > copy up has not taken place yet. > > > > And if CPU1 needed to do use upper dentry only, then it will do flags=WRITE > > and that will take oi->lock and make sure OVL_UPPER_DATA is set. > > > > So only *additional* smp_rmb()/smp_wmb() we require for the case when > > data is copied up later and we need to make sure OVL_UPPER_DATA is > > visible only after the full data copy up is done and stable. > > > > > > Right. forgot about that wmb. > > >> > >> So IMO you may only need to add smp_rmb() before > >> ovl_test_flag(OVL_UPPER_DATA in ovl_d_real() and the smp_wmb() > >> in ovl_inode_update() should be sufficient. > >> Change the comment in ovl_inode_update() to mention that wmb also > >> matches rmb in ovl_d_real() w.r.t OVL_UPPER_DATA flag. > > > > Hmm..., I agree that we require smp_rmb() here but it will pair with > > smp_wmb() in ovl_copy_meta_data_inode() and not the one in > > ovl_inode_update(), right? Something like. > > Right. my bad. > > > > > ovl_d_real() { > > bool has_upper_data; > > > > has_upper_data = ovl_test_flag(OVL_UPPER_DATA, d_inode(dentry)); > > /* Pairs with smp_wmb() in ovl_copy_up_meta_inode_data() */ > > smp_rmb(); > > if (!has_upper_data) > > goto lower; > > Just put smp_rmb() here. no need for the bool variable. > rmb does matter if you goto lower... I thought smp_rmb() has to be put *only* after LOAD of oi->flags. Something like. LOAD oi->flags smp_rmb() Look at results of oi->flags and take action. So that means I need to store results of oi->flags load in variable temporarily so that I can analyze it after smp_rmb(). IOW, I am not sure how would I get rid of boolean here. I need some kind of temp variable. Vivek > > > > > ... > > ... > > return real; > > } > > > > Note that now smp_rmb() will be placed after loading OVL_UPPER_DATA and > > not before it. Because we are ensuring ordering w.r.t smp_wmb() in > > ovl_copy_up_meta_inode_data(). > > > > In fact I think my current patches are buggy because they should have had > > this smp_rmb() to begin with. > >