Messages in this thread Patch in this message | | | Date | Tue, 10 May 2016 16:20:16 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 5/9] staging: lustre: fid: packing ost_idx in IDIF |
| |
On Wed, May 04, 2016 at 10:28:56AM -0400, James Simmons wrote: > From: Fan Yong <fan.yong@intel.com> > > For a normal FID, we can know on which target the related object > is allocated via querying FLDB; but it is not true for an IDIF. > > To locate the OST via the given IDIF, when the IDIF is generated, > we pack the OST index in it. Then for any given FID, in spite of > t is a normal FID or not, we has the method to know which target
Missing words. "in spite of *whether it* is a normal FID or not."
> it belongs to. That is useful for LFSCK. > > For old IDIF, the OST index is not part of the IDIF, means that > ifferent OSTs may have the same IDIFs, that may cause the IFID
different.
> in LMA does not match the read FID.
s/does/to/
> Under such case, we need to > make some compatible check to avoid to trigger unexpected. > > tgt_validate_obdo() converts the ostid contained in the RPC body > to fid and changes the "struct ost_id" union, then the users can > access ost_id::oi_fid directly without call ostid_to_fid() again.
calling.
> > It also contains some other fixing and cleanup.
These are trigger words to avoid.
> > Signed-off-by: wang di <di.wang@intel.com> > Signed-off-by: Fan Yong <fan.yong@intel.com> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-3569 > Reviewed-on: http://review.whamcloud.com/7053 > Reviewed-by: Andreas Dilger <andreas.dilger@intel.com> > Reviewed-by: Alex Zhuravlev <alexey.zhuravlev@intel.com> > Signed-off-by: James Simmons <jsimmons@infradead.org> > --- > .../lustre/lustre/include/lustre/lustre_idl.h | 76 +++++++++++++++------- > drivers/staging/lustre/lustre/include/lustre_fid.h | 22 ++----- > 2 files changed, 57 insertions(+), 41 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h > index a70545a..9c53c17 100644 > --- a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h > +++ b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h > @@ -584,7 +584,7 @@ static inline __u64 ostid_seq(const struct ost_id *ostid) > if (fid_seq_is_mdt0(ostid->oi.oi_seq)) > return FID_SEQ_OST_MDT0; > > - if (fid_seq_is_default(ostid->oi.oi_seq)) > + if (unlikely(fid_seq_is_default(ostid->oi.oi_seq)))
Adding unlikely() is the opposite of a cleanup. It's now messier. That sort of thing needs to be justified by benchmarks. Please remove all the new likely/unlikelys and add them in a separate patch later.
> return FID_SEQ_LOV_DEFAULT; > > if (fid_is_idif(&ostid->oi_fid)) > @@ -596,9 +596,12 @@ static inline __u64 ostid_seq(const struct ost_id *ostid) > /* extract OST objid from a wire ost_id (id/seq) pair */ > static inline __u64 ostid_id(const struct ost_id *ostid) > { > - if (fid_seq_is_mdt0(ostid_seq(ostid))) > + if (fid_seq_is_mdt0(ostid->oi.oi_seq)) > return ostid->oi.oi_id & IDIF_OID_MASK; > > + if (unlikely(fid_seq_is_default(ostid->oi.oi_seq))) > + return ostid->oi.oi_id; > + > if (fid_is_idif(&ostid->oi_fid)) > return fid_idif_id(fid_seq(&ostid->oi_fid), > fid_oid(&ostid->oi_fid), 0); > @@ -642,12 +645,22 @@ static inline void ostid_set_seq_llog(struct ost_id *oi) > */ > static inline void ostid_set_id(struct ost_id *oi, __u64 oid) > { > - if (fid_seq_is_mdt0(ostid_seq(oi))) { > + if (fid_seq_is_mdt0(oi->oi.oi_seq)) { > if (oid >= IDIF_MAX_OID) { > CERROR("Bad %llu to set " DOSTID "\n", oid, POSTID(oi)); > return; > } > oi->oi.oi_id = oid; > + } else if (fid_is_idif(&oi->oi_fid)) { > + if (oid >= IDIF_MAX_OID) { > + CERROR("Bad %llu to set "DOSTID"\n", > + oid, POSTID(oi)); > + return; > + } > + oi->oi_fid.f_seq = fid_idif_seq(oid, > + fid_idif_ost_idx(&oi->oi_fid)); > + oi->oi_fid.f_oid = oid; > + oi->oi_fid.f_ver = oid >> 48; > } else { > if (oid > OBIF_MAX_OID) { > CERROR("Bad %llu to set " DOSTID "\n", oid, POSTID(oi)); > @@ -657,25 +670,31 @@ static inline void ostid_set_id(struct ost_id *oi, __u64 oid) > } > } > > -static inline void ostid_inc_id(struct ost_id *oi) > +static inline int fid_set_id(struct lu_fid *fid, __u64 oid) > { > - if (fid_seq_is_mdt0(ostid_seq(oi))) { > - if (unlikely(ostid_id(oi) + 1 > IDIF_MAX_OID)) { > - CERROR("Bad inc "DOSTID"\n", POSTID(oi)); > - return; > + if (unlikely(fid_seq_is_igif(fid->f_seq))) { > + CERROR("bad IGIF, "DFID"\n", PFID(fid)); > + return -EBADF; > + } > + > + if (fid_is_idif(fid)) { > + if (oid >= IDIF_MAX_OID) { > + CERROR("Too large OID %#llx to set IDIF "DFID"\n", > + (unsigned long long)oid, PFID(fid)); > + return -EBADF; > } > - oi->oi.oi_id++; > + fid->f_seq = fid_idif_seq(oid, fid_idif_ost_idx(fid)); > + fid->f_oid = oid; > + fid->f_ver = oid >> 48; > } else { > - oi->oi_fid.f_oid++; > + if (oid > OBIF_MAX_OID) {
Is this off by one? Hopely it is. Otherwise, it's really confusing.
> + CERROR("Too large OID %#llx to set REG "DFID"\n", > + (unsigned long long)oid, PFID(fid)); > + return -EBADF; > + } > + fid->f_oid = oid; > }
regards, dan carpenter
| |