Whamcloud - gitweb
LU-4239 mdt: Optimize fid2path 17/10717/19
authorPatrick Farrell <paf@cray.com>
Mon, 1 Dec 2014 06:56:18 +0000 (00:56 -0600)
committerOleg Drokin <oleg.drokin@intel.com>
Fri, 26 Dec 2014 16:58:23 +0000 (16:58 +0000)
On directory paths 100 or more deep, fid2path
gives a spurious:
ioctl err -75: Value too large for defined data type

This bug occurs because all of the fids found during
a path lookup are stored in pli_fids and the
array is too small for very deep paths.

The stored fids are not used except for the last one, so
this array is not necessary.

Additionally, from Frank Zago:

It is not necessary to copy back and forth the fid, linkno,
and path.
Keep the input getinfo_fid2path, and remove

struct path_lookup_info

which was mostly a copy of getinfo_fid2path. This saves
several copies and a memory allocation.

The fid to be returned back to user space is stored in
fp->gf_fid.

Also added doxygen comments and replaced an 'EXIT' and
'return rc' with 'RETURN(rc)'.

Signed-off-by: Patrick Farrell <paf@cray.com>
Signed-off-by: Frank Zago <fzago@cray.com>
Change-Id: I357b4d91b9c65a836d289d5815c763306ac34565
Reviewed-on: http://review.whamcloud.com/10717
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: James Simmons <uja.ornl@gmail.com>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
lustre/mdt/mdt_handler.c
lustre/tests/sanity.sh

index 79ca02b..4f5ec9e 100644 (file)
@@ -5322,25 +5322,6 @@ static int mdt_destroy_export(struct obd_export *exp)
        RETURN(0);
 }
 
-/** The maximum depth that fid2path() will search.
- * This is limited only because we want to store the fids for
- * historical path lookup purposes.
- */
-#define MAX_PATH_DEPTH 100
-
-/** mdt_path() lookup structure. */
-struct path_lookup_info {
-       __u64                   pli_recno;      /**< history point */
-       __u64                   pli_currec;     /**< current record */
-       struct lu_fid           pli_fid;
-       struct lu_fid           pli_fids[MAX_PATH_DEPTH]; /**< path, in fids */
-       struct mdt_object       *pli_mdt_obj;
-       char                    *pli_path;      /**< full path */
-       int                     pli_pathlen;
-       int                     pli_linkno;     /**< which hardlink to follow */
-       int                     pli_fidcount;   /**< number of \a pli_fids */
-};
-
 int mdt_links_read(struct mdt_thread_info *info, struct mdt_object *mdt_obj,
                   struct linkea_data *ldata)
 {
@@ -5372,8 +5353,21 @@ int mdt_links_read(struct mdt_thread_info *info, struct mdt_object *mdt_obj,
        return linkea_init(ldata);
 }
 
+/**
+ * Given an MDT object, try to look up the full path to the object.
+ * Part of the MDT layer implementation of lfs fid2path.
+ *
+ * \param[in]     info  Per-thread common data shared by MDT level handlers.
+ * \param[in]     obj   Object to do path lookup of
+ * \param[in,out] fp    User-provided struct to store path information
+ *
+ * \retval 0 Lookup successful, path information stored in fp
+ * \retval -EAGAIN Lookup failed, usually because object is being moved
+ * \retval negative errno if there was a problem
+ */
 static int mdt_path_current(struct mdt_thread_info *info,
-                           struct path_lookup_info *pli)
+                           struct mdt_object *obj,
+                           struct getinfo_fid2path *fp)
 {
        struct mdt_device       *mdt = info->mti_mdt;
        struct mdt_object       *mdt_obj;
@@ -5386,6 +5380,7 @@ static int mdt_path_current(struct mdt_thread_info *info,
        int                     reclen;
        struct linkea_data      ldata = { 0 };
        int                     rc = 0;
+       bool                    first = true;
        ENTRY;
 
        /* temp buffer for path element, the buffer will be finally freed
@@ -5395,16 +5390,14 @@ static int mdt_path_current(struct mdt_thread_info *info,
                RETURN(-ENOMEM);
 
        ldata.ld_buf = buf;
-       ptr = pli->pli_path + pli->pli_pathlen - 1;
+       ptr = fp->gf_path + fp->gf_pathlen - 1;
        *ptr = 0;
        --ptr;
-       pli->pli_fidcount = 0;
-       pli->pli_fids[0] = *(struct lu_fid *)mdt_object_fid(pli->pli_mdt_obj);
-       *tmpfid = pli->pli_fids[0];
+       *tmpfid = fp->gf_fid = *mdt_object_fid(obj);
+
        /* root FID only exists on MDT0, and fid2path should also ends at MDT0,
         * so checking root_fid can only happen on MDT0. */
-       while (!lu_fid_eq(&mdt->mdt_md_root_fid,
-                         &pli->pli_fids[pli->pli_fidcount])) {
+       while (!lu_fid_eq(&mdt->mdt_md_root_fid, &fp->gf_fid)) {
                struct lu_buf           lmv_buf;
 
                mdt_obj = mdt_object_find(info->mti_env, mdt, tmpfid);
@@ -5432,18 +5425,17 @@ static int mdt_path_current(struct mdt_thread_info *info,
                linkea_entry_unpack(lee, &reclen, tmpname, tmpfid);
                /* If set, use link #linkno for path lookup, otherwise use
                   link #0.  Only do this for the final path element. */
-               if (pli->pli_fidcount == 0 &&
-                   pli->pli_linkno < leh->leh_reccount) {
+               if (first && fp->gf_linkno < leh->leh_reccount) {
                        int count;
-                       for (count = 0; count < pli->pli_linkno; count++) {
+                       for (count = 0; count < fp->gf_linkno; count++) {
                                lee = (struct link_ea_entry *)
                                     ((char *)lee + reclen);
                                linkea_entry_unpack(lee, &reclen, tmpname,
                                                    tmpfid);
                        }
-                       if (pli->pli_linkno < leh->leh_reccount - 1)
+                       if (fp->gf_linkno < leh->leh_reccount - 1)
                                /* indicate to user there are more links */
-                               pli->pli_linkno++;
+                               fp->gf_linkno++;
                }
 
                lmv_buf.lb_buf = info->mti_xattr_buf;
@@ -5457,7 +5449,7 @@ static int mdt_path_current(struct mdt_thread_info *info,
 
                        /* For slave stripes, get its master */
                        if (le32_to_cpu(lmm->lmv_magic) == LMV_MAGIC_STRIPE) {
-                               pli->pli_fids[pli->pli_fidcount] = *tmpfid;
+                               fp->gf_fid = *tmpfid;
                                continue;
                        }
                } else if (rc < 0 && rc != -ENODATA) {
@@ -5468,72 +5460,81 @@ static int mdt_path_current(struct mdt_thread_info *info,
 
                /* Pack the name in the end of the buffer */
                ptr -= tmpname->ln_namelen;
-               if (ptr - 1 <= pli->pli_path)
+               if (ptr - 1 <= fp->gf_path)
                        GOTO(out, rc = -EOVERFLOW);
                strncpy(ptr, tmpname->ln_name, tmpname->ln_namelen);
                *(--ptr) = '/';
 
-               /* Store the parent fid for historic lookup */
-               if (++pli->pli_fidcount >= MAX_PATH_DEPTH)
-                       GOTO(out, rc = -EOVERFLOW);
-               pli->pli_fids[pli->pli_fidcount] = *tmpfid;
+               /* keep the last resolved fid to the client, so the
+                * client will build the left path on another MDT for
+                * remote object */
+               fp->gf_fid = *tmpfid;
+
+               first = false;
        }
 
 remote_out:
        ptr++; /* skip leading / */
-       memmove(pli->pli_path, ptr, pli->pli_path + pli->pli_pathlen - ptr);
+       memmove(fp->gf_path, ptr, fp->gf_path + fp->gf_pathlen - ptr);
 
-       EXIT;
 out:
-       return rc;
+       RETURN(rc);
 }
 
-/* Returns the full path to this fid, as of changelog record recno. */
+/**
+ * Given an MDT object, use mdt_path_current to get the path.
+ * Essentially a wrapper to retry mdt_path_current a set number of times
+ * if -EAGAIN is returned (usually because an object is being moved).
+ *
+ * Part of the MDT layer implementation of lfs fid2path.
+ *
+ * \param[in]     info  Per-thread common data shared by mdt level handlers.
+ * \param[in]     obj   Object to do path lookup of
+ * \param[in,out] fp    User-provided struct for arguments and to store path
+ *                     information
+ *
+ * \retval 0 Lookup successful, path information stored in fp
+ * \retval negative errno if there was a problem
+ */
 static int mdt_path(struct mdt_thread_info *info, struct mdt_object *obj,
-                   char *path, int pathlen, __u64 *recno, int *linkno,
-                   struct lu_fid *fid)
+                   struct getinfo_fid2path *fp)
 {
        struct mdt_device       *mdt = info->mti_mdt;
-       struct path_lookup_info *pli;
        int                     tries = 3;
        int                     rc = -EAGAIN;
        ENTRY;
 
-       if (pathlen < 3)
+       if (fp->gf_pathlen < 3)
                RETURN(-EOVERFLOW);
 
        if (lu_fid_eq(&mdt->mdt_md_root_fid, mdt_object_fid(obj))) {
-               path[0] = '\0';
+               fp->gf_path[0] = '\0';
                RETURN(0);
        }
 
-       OBD_ALLOC_PTR(pli);
-       if (pli == NULL)
-               RETURN(-ENOMEM);
-
-       pli->pli_mdt_obj = obj;
-       pli->pli_recno = *recno;
-       pli->pli_path = path;
-       pli->pli_pathlen = pathlen;
-       pli->pli_linkno = *linkno;
-
        /* Retry multiple times in case file is being moved */
        while (tries-- && rc == -EAGAIN)
-               rc = mdt_path_current(info, pli);
-
-       /* return the last resolved fids to the client, so the client will
-        * build the left path on another MDT for remote object */
-       *fid = pli->pli_fids[pli->pli_fidcount];
-
-       *recno = pli->pli_currec;
-       /* Return next link index to caller */
-       *linkno = pli->pli_linkno;
-
-       OBD_FREE_PTR(pli);
+               rc = mdt_path_current(info, obj, fp);
 
        RETURN(rc);
 }
 
+/**
+ * Get the full path of the provided FID, as of changelog record recno.
+ *
+ * This checks sanity and looks up object for user provided FID
+ * before calling the actual path lookup code.
+ *
+ * Part of the MDT layer implementation of lfs fid2path.
+ *
+ * \param[in]     info  Per-thread common data shared by mdt level handlers.
+ * \param[in,out] fp    User-provided struct for arguments and to store path
+ *                     information
+ *
+ * \retval 0 Lookup successful, path information and recno stored in fp
+ * \retval -ENOENT, object does not exist
+ * \retval negative errno if there was a problem
+ */
 static int mdt_fid2path(struct mdt_thread_info *info,
                        struct getinfo_fid2path *fp)
 {
@@ -5576,8 +5577,7 @@ static int mdt_fid2path(struct mdt_thread_info *info,
                RETURN(rc);
        }
 
-       rc = mdt_path(info, obj, fp->gf_path, fp->gf_pathlen, &fp->gf_recno,
-                     &fp->gf_linkno, &fp->gf_fid);
+       rc = mdt_path(info, obj, fp);
 
        CDEBUG(D_INFO, "fid "DFID", path %s recno "LPX64" linkno %u\n",
               PFID(&fp->gf_fid), fp->gf_path, fp->gf_recno, fp->gf_linkno);
index 66c9642..cd2f4da 100644 (file)
@@ -10532,6 +10532,8 @@ check_path() {
     local fid=$2
 
     local path=$(${LFS} fid2path $*)
+    # Remove the '//' indicating a remote directory
+    path=$(echo $path | sed 's#//#/#g')
     RC=$?
 
     if [ $RC -ne 0 ]; then
@@ -10626,6 +10628,33 @@ test_162b() {
 }
 run_test 162b "striped directory path lookup sanity"
 
+# LU-4239: Verify fid2path works with paths 100 or more directories deep
+test_162c() {
+       test_mkdir $DIR/$tdir.local
+       test_mkdir $DIR/$tdir.remote
+       local lpath=$tdir.local
+       local rpath=$tdir.remote
+
+       for ((i = 0; i <= 101; i++)); do
+               lpath="$lpath/$i"
+               mkdir $DIR/$lpath
+               FID=$($LFS path2fid $DIR/$lpath | tr -d '[]') ||
+                       error "get fid for local directory $DIR/$lpath failed"
+               check_path "$DIR/$lpath" $MOUNT $FID --link 0 ||
+                       error "check path for local directory $DIR/$lpath failed"
+
+               rpath="$rpath/$i"
+               test_mkdir $DIR/$rpath
+               FID=$($LFS path2fid $DIR/$rpath | tr -d '[]') ||
+                       error "get fid for remote directory $DIR/$rpath failed"
+               check_path "$DIR/$rpath" $MOUNT $FID --link 0 ||
+                       error "check path for remote directory $DIR/$rpath failed"
+       done
+
+       return 0
+}
+run_test 162c "fid2path works with paths 100 or more directories deep"
+
 test_169() {
        # do directio so as not to populate the page cache
        log "creating a 10 Mb file"