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.

Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>
Change-Id: I4bd37395a8eff94458e62414a5a4e7206b76d32d
Reviewed-on: http://review.whamcloud.com/10179
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: James Nunez <james.a.nunez@intel.com>
Reviewed-by: Aurelien Degremont <aurelien.degremont@cea.fr>
Reviewed-by: Faccini Bruno <bruno.faccini@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>

diff --git a/lustre/mdd/mdd_dir.c b/lustre/mdd/mdd_dir.c
index d1009a3..9244db3 100644 (file)
--- a/lustre/mdd/mdd_dir.c
+++ b/lustre/mdd/mdd_dir.c
@@ -1029,7 +1029,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;
 
@@ -1047,9 +1046,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;
@@ -1084,10 +1081,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,
@@ -2011,7 +2008,7 @@ static int mdd_declare_create(const struct lu_env *env, struct mdd_device *mdd,
                        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);
                if (rc)
                        GOTO(out, rc);
@@ -2031,11 +2028,11 @@ 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;
+       }
 
        /* XXX: For remote create, it should indicate the remote RPC
         * will be sent after local transaction is finished, which
@@ -2314,9 +2311,7 @@ static int mdd_create(const struct lu_env *env, struct md_object *pobj,
        if (unlikely(spec->sp_cr_flags & MDS_OPEN_VOLATILE)) {
                mdd_write_lock(env, son, MOR_TGT_CHILD);
                rc = __mdd_orphan_add(env, son, handle);
-               mdd_write_unlock(env, son);
-               if (rc != 0)
-                       GOTO(err_created, rc);
+               GOTO(out_volatile, rc);
        } else {
                rc = __mdd_index_insert(env, mdd_pobj, mdo2fid(son),
                                        name, S_ISDIR(attr->la_mode), handle,
@@ -2353,29 +2348,37 @@ err_created:
                mdd_write_lock(env, son, MOR_TGT_CHILD);
                if (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:
        rc2 = mdd_trans_stop(env, mdd, rc, handle);
        if (rc == 0)
@@ -2385,11 +2388,11 @@ out_free:
                /* if we vmalloced a large buffer drop it */
                lu_buf_free(ldata->ld_buf);
 
-        /* The child object shouldn't be cached anymore */
-        if (rc)
+       /* The child object shouldn't be cached anymore */
+       if (rc)
                set_bit(LU_OBJECT_HEARD_BANSHEE,
-                            &child->mo_lu.lo_header->loh_flags);
-        return rc;
+                       &child->mo_lu.lo_header->loh_flags);
+       return rc;
 }
 
 /*

diff --git a/lustre/utils/liblustreapi.c b/lustre/utils/liblustreapi.c
index 6b06ebb..56804b8 100644 (file)
--- a/lustre/utils/liblustreapi.c
+++ b/lustre/utils/liblustreapi.c
@@ -4431,14 +4431,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.
@@ -4480,11 +4487,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;
 }