| Message ID | 1537205440-6656-18-git-send-email-jsimmons@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lustre: first batch of fixes from lustre 2.10 | expand |
On Mon, Sep 17 2018, James Simmons wrote: > From: Andriy Skulysh <c17819@cray.com> > > The ll_dentry_data bitfields modification should be protected by > a spinlock. > > Signed-off-by: Andriy Skulysh <c17819@cray.com> > Signed-off-by: Lai Siyao <lai.siyao@whamcloud.com> > WC-bug-id: https://jira.whamcloud.com/browse/LU-9241 > Seagate-bug-id: MRP-4257 > Reviewed-on: https://review.whamcloud.com/26109 > Reviewed-by: Niu Yawei <yawei.niu@intel.com> > Reviewed-by: Bobi Jam <bobijam@hotmail.com> > Reviewed-by: Oleg Drokin <green@whamcloud.com> > Signed-off-by: James Simmons <jsimmons@infradead.org> > --- > drivers/staging/lustre/lustre/llite/llite_nfs.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/lustre/lustre/llite/llite_nfs.c b/drivers/staging/lustre/lustre/llite/llite_nfs.c > index c66072a..5e91e83 100644 > --- a/drivers/staging/lustre/lustre/llite/llite_nfs.c > +++ b/drivers/staging/lustre/lustre/llite/llite_nfs.c > @@ -171,8 +171,9 @@ struct lustre_nfs_fid { > * we came from NFS and so opencache needs to be > * enabled for this one > */ > + spin_lock(&result->d_lock); > ll_d2d(result)->lld_nfs_dentry = 1; > - > + spin_unlock(&result->d_lock); > return result; > } This is a bit weird.... I agree that having a spinlock is needed as the compiler does an R/M/W to update the bitfield, and that could race with updats to lld_invalid (and I think that is a good arguement to avoid bitfields in shared structures). However lld_nfs_dentry is never used. The only use was removed by Commit: c1b66fccf986 ("staging: lustre: fid: do open-by-fid by default") The corresponding commit upstream is Commit: cb85c0364fd8 ("LU-3544 fid: do open-by-fid by default") and that commit doesn't remove the use of lld_nfs_dentry. Maybe because lld_nfs_dentry didn't exist then - it wasn't added until later by 65d0add6057b138. So do we need to bring lld_nfs_dentry back? Thanks, NeilBrown
> > > From: Andriy Skulysh <c17819@cray.com> > > > > The ll_dentry_data bitfields modification should be protected by > > a spinlock. > > > > Signed-off-by: Andriy Skulysh <c17819@cray.com> > > Signed-off-by: Lai Siyao <lai.siyao@whamcloud.com> > > WC-bug-id: https://jira.whamcloud.com/browse/LU-9241 > > Seagate-bug-id: MRP-4257 > > Reviewed-on: https://review.whamcloud.com/26109 > > Reviewed-by: Niu Yawei <yawei.niu@intel.com> > > Reviewed-by: Bobi Jam <bobijam@hotmail.com> > > Reviewed-by: Oleg Drokin <green@whamcloud.com> > > Signed-off-by: James Simmons <jsimmons@infradead.org> > > --- > > drivers/staging/lustre/lustre/llite/llite_nfs.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/lustre/lustre/llite/llite_nfs.c b/drivers/staging/lustre/lustre/llite/llite_nfs.c > > index c66072a..5e91e83 100644 > > --- a/drivers/staging/lustre/lustre/llite/llite_nfs.c > > +++ b/drivers/staging/lustre/lustre/llite/llite_nfs.c > > @@ -171,8 +171,9 @@ struct lustre_nfs_fid { > > * we came from NFS and so opencache needs to be > > * enabled for this one > > */ > > + spin_lock(&result->d_lock); > > ll_d2d(result)->lld_nfs_dentry = 1; > > - > > + spin_unlock(&result->d_lock); > > return result; > > } > > This is a bit weird.... > I agree that having a spinlock is needed as the compiler does an R/M/W > to update the bitfield, and that could race with updats to lld_invalid > (and I think that is a good arguement to avoid bitfields in shared structures). > > However lld_nfs_dentry is never used. The only use was removed by > > Commit: c1b66fccf986 ("staging: lustre: fid: do open-by-fid by default") > > The corresponding commit upstream is > > Commit: cb85c0364fd8 ("LU-3544 fid: do open-by-fid by default") > > and that commit doesn't remove the use of lld_nfs_dentry. Maybe because > lld_nfs_dentry didn't exist then - it wasn't added until later > by 65d0add6057b138. > > So do we need to bring lld_nfs_dentry back? Ouch. That got removed by mistake by me ;-( Yes it should come back. I will create a patch to fix that. My bad.
diff --git a/drivers/staging/lustre/lustre/llite/llite_nfs.c b/drivers/staging/lustre/lustre/llite/llite_nfs.c index c66072a..5e91e83 100644 --- a/drivers/staging/lustre/lustre/llite/llite_nfs.c +++ b/drivers/staging/lustre/lustre/llite/llite_nfs.c @@ -171,8 +171,9 @@ struct lustre_nfs_fid { * we came from NFS and so opencache needs to be * enabled for this one */ + spin_lock(&result->d_lock); ll_d2d(result)->lld_nfs_dentry = 1; - + spin_unlock(&result->d_lock); return result; }