LU-3696 mdd: decref volatile object after creation

Drop the nlink count on a volatile file after it is created.
It is created with a single reference and then added to the PENDING
directory, which adds a second reference to the inode.  When it is
closed it is left with an nlink count=1 and not unlinked as it should
be.

Do not insert volatile files into the ChangeLog, since they are
intended to be temporary files that cannot be accessed after creation
and should not be synched to a backup filesystem.

If llapi_create_volatile_idx() is used on a non-Lustre filesystem then
it should also unlink the temporary filename after opening it, so that
the behaviour is the same.  Improve the comments for this function.

Minor cleanups of related functions found during investigation.

This patch is based on Andreas Dilger's patch:
Lustre-change: http://review.whamcloud.com/#/c/10179
Lustre-commit: 1ebc8dc473dd9f948837b9d5b258194637d46525

Signed-off-by: James Nunez <james.a.nunez@intel.com>
Change-Id: Ief906362ab2f6f92342c8eabcb9b090a340a5003
Reviewed-on: http://review.whamcloud.com/12076
Tested-by: Jenkins
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Tested-by: Maloo <hpdd-maloo@intel.com>

diff --git a/lustre/mdd/mdd_dir.c b/lustre/mdd/mdd_dir.c
index 33a4d1c..c36d478 100644 (file)
--- a/lustre/mdd/mdd_dir.c
+++ b/lustre/mdd/mdd_dir.c
@@ -990,7 +990,6 @@ int mdd_links_rename(const struct lu_env *env,
                     struct linkea_data *ldata,
                     int first, int check)
 {
-       int rc2 = 0;
        int rc = 0;
        ENTRY;
 
@@ -1008,9 +1007,7 @@ int mdd_links_rename(const struct lu_env *env,
                rc = mdd_links_write(env, mdd_obj, ldata, handle);
        EXIT;
 out:
-       if (rc == 0)
-               rc = rc2;
-       if (rc) {
+       if (rc != 0) {
                int error = 1;
                if (rc == -EOVERFLOW || rc == -ENOSPC)
                        error = 0;
@@ -1045,10 +1042,10 @@ static inline int mdd_links_add(const struct lu_env *env,
                                const struct lu_fid *pfid,
                                const struct lu_name *lname,
                                struct thandle *handle,
-                               struct linkea_data *data, int first)
+                               struct linkea_data *ldata, int first)
 {
        return mdd_links_rename(env, mdd_obj, NULL, NULL,
-                               pfid, lname, handle, data, first, 0);
+                               pfid, lname, handle, ldata, first, 0);
 }
 
 static inline int mdd_links_del(const struct lu_env *env,
@@ -1891,7 +1888,7 @@ static int mdd_declare_create(const struct lu_env *env, struct mdd_device *mdd,
        if (rc)
                GOTO(out, rc);
 
-       if (spec->sp_cr_flags & MDS_OPEN_VOLATILE)
+       if (unlikely(spec->sp_cr_flags & MDS_OPEN_VOLATILE))
                rc = orph_declare_index_insert(env, c, attr->la_mode, handle);
        else
                rc = mdo_declare_index_insert(env, p, mdo2fid(c),
@@ -1927,14 +1924,14 @@ static int mdd_declare_create(const struct lu_env *env, struct mdd_device *mdd,
                rc = mdo_declare_attr_set(env, p, la, handle);
                if (rc)
                        return rc;
-       }
 
-        rc = mdd_declare_changelog_store(env, mdd, name, handle);
-        if (rc)
-                return rc;
+               rc = mdd_declare_changelog_store(env, mdd, name, handle);
+               if (rc)
+                       return rc;
+       }
 
 out:
-        return rc;
+       return rc;
 }
 
 static int mdd_acl_init(const struct lu_env *env, struct mdd_object *pobj,
@@ -2127,8 +2124,10 @@ static int mdd_create(const struct lu_env *env, struct md_object *pobj,
                                BYPASS_CAPA);
        }
 
-       if (rc == 0 && spec->sp_cr_flags & MDS_OPEN_VOLATILE)
+       if (rc == 0 && spec->sp_cr_flags & MDS_OPEN_VOLATILE){
                rc = __mdd_orphan_add(env, son, handle);
+               GOTO(out_volatile, rc);
+       }
 
        mdd_write_unlock(env, son);
 
@@ -2172,8 +2171,10 @@ static int mdd_create(const struct lu_env *env, struct md_object *pobj,
         }
 
        /* volatile file creation does not update parent directory times */
-       if (spec->sp_cr_flags & MDS_OPEN_VOLATILE)
-               GOTO(cleanup, rc = 0);
+       if (unlikely(spec->sp_cr_flags & MDS_OPEN_VOLATILE)) {
+               mdd_write_lock(env, son, MOR_TGT_CHILD);
+               GOTO(out_volatile, rc = 0);
+       }
 
        /* update parent directory mtime/ctime */
        *la = *attr;
@@ -2201,32 +2202,39 @@ cleanup:
                mdd_write_lock(env, son, MOR_TGT_CHILD);
                if (initialized != 0 && S_ISDIR(attr->la_mode)) {
                        /* Drop the reference, no need to delete "."/"..",
-                        * because the object to be destroied directly. */
+                        * because the object is to be destroyed directly. */
                        rc2 = mdo_ref_del(env, son, handle);
                        if (rc2 != 0) {
                                mdd_write_unlock(env, son);
                                goto out_stop;
                        }
                }
-
+out_volatile:
+               /* For volatile files drop one link immediately, since there is
+                * no filename in the namespace, and save any error returned. */
                rc2 = mdo_ref_del(env, son, handle);
                if (rc2 != 0) {
                        mdd_write_unlock(env, son);
+                       if(unlikely(rc == 0))
+                               rc = rc2;
                        goto out_stop;
                }
 
-               mdo_destroy(env, son, handle);
+               /* Don't destroy the volatile object on success */
+               if (likely(rc != 0))
+                       mdo_destroy(env, son, handle);
                mdd_write_unlock(env, son);
-        }
+       }
 
-       if (rc == 0 && fid_is_namespace_visible(mdo2fid(son)))
+       if (rc == 0 && fid_is_namespace_visible(mdo2fid(son)) &&
+           likely((spec->sp_cr_flags & MDS_OPEN_VOLATILE) == 0))
                rc = mdd_changelog_ns_store(env, mdd,
-                       S_ISDIR(attr->la_mode) ? CL_MKDIR :
-                       S_ISREG(attr->la_mode) ? CL_CREATE :
-                       S_ISLNK(attr->la_mode) ? CL_SOFTLINK : CL_MKNOD,
-                       0, son, mdd_pobj, lname, handle);
+                               S_ISDIR(attr->la_mode) ? CL_MKDIR :
+                               S_ISREG(attr->la_mode) ? CL_CREATE :
+                               S_ISLNK(attr->la_mode) ? CL_SOFTLINK : CL_MKNOD,
+                               0, son, mdd_pobj, lname, handle);
 out_stop:
-        mdd_trans_stop(env, mdd, rc, handle);
+       mdd_trans_stop(env, mdd, rc, handle);
 out_free:
        if (ldata->ld_buf && ldata->ld_buf->lb_len > OBD_ALLOC_BIG)
                /* if we vmalloced a large buffer drop it */

diff --git a/lustre/utils/liblustreapi.c b/lustre/utils/liblustreapi.c
index 90f28fe..09beb32 100644 (file)
--- a/lustre/utils/liblustreapi.c
+++ b/lustre/utils/liblustreapi.c
@@ -4308,14 +4308,21 @@ int llapi_get_data_version(int fd, __u64 *data_version, __u64 flags)
 }
 
 /*
- * Create a volatile file and open it for write:
- * - file is created as a standard file in the directory
- * - file does not appears in directory and directory mtime does not change
- * - file is removed at close
- * - file modes are rw-------, if user wants another one it must use fchmod()
- * \param      directory       Directory where the file is created
- * \param      idx             MDT index on which the file is created
- * \param      open_flags      Standard open flags
+ * Create a file without any name open it for read/write:
+ *
+ * - file is created as if it were a standard file in the given \a directory
+ * - file does not appear in \a directory and mtime does not change because
+ *   the filename is handled specially by the Lustre MDS.
+ * - file is removed at final close
+ * - file modes are rw-------, since it doesn't make sense to have a read-only
+ *   or write-only file that cannot be opened again.
+ * - if user wants another mode it must use fchmod() on the open file, no
+ *   security problems arise because it cannot be opened by another process.
+ *
+ * \param[in]  directory       directory from which to inherit layout/MDT idx
+ * \param[in]  idx             MDT index on which the file is created,
+ *                             \a idx == -1 means no specific MDT is requested
+ * \param[in]  open_flags      standard open(2) flags
  *
  * \retval     0 on success.
  * \retval     -errno on error.
@@ -4357,11 +4364,16 @@ int llapi_create_volatile_idx(char *directory, int idx, int open_flags)
        fd = open(file_path, O_RDWR | O_CREAT | open_flags, S_IRUSR | S_IWUSR);
        if (fd < 0) {
                llapi_error(LLAPI_MSG_ERROR, errno,
-                           "Cannot create volatile file %s in %s\n",
+                           "Cannot create volatile file '%s' in '%s'\n",
                            filename + LUSTRE_VOLATILE_HDR_LEN,
                            directory);
                return -errno;
        }
+       /* unlink file in case this wasn't a Lustre filesystem, and the
+        * magic volatile filename wasn't handled as intended. The effect
+        * is the same. */
+       unlink(file_path);
+
        return fd;
 }