lkml.org 
[lkml]   [2016]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2016-05-10 15:41    [W:0.507 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site