Whamcloud - gitweb
LU-11832 ldiskfs: properly handle VFS parallel locking 14/34714/30
authorJames Simmons <jsimmons@infradead.org>
Thu, 21 Nov 2019 22:15:10 +0000 (17:15 -0500)
committerOleg Drokin <green@whamcloud.com>
Sat, 14 Dec 2019 05:56:51 +0000 (05:56 +0000)
The lustre server stack has an OSD abstraction to treat the under
lying native file system as an object store. In this case it is
ext4 being used to store internal data the OSD abstraction uses.

The area the recent VFS parallel locking changes impacts us is in
the LFSCK scrubbing code. The method the scrubbing code uses to
ensure a healthy state for the OSD data that is stored is by
using the native file system backend directory scanning code which
is setup to pass in unique function hook to handle each OSD
object store data file.

Currently this starts with the scrub code calling code that resembles
the internals of iterate_dir() which in turn calls ext4_readdir() with
struct dir_context actor mapping to each unique callback defined
by the field olm_filldir of struct osd_lf_map. The inode_lock()
and inode_unlock() was called with osd_lookup_one_len_unlocked()
which is often present in the dir_context actor function as defined by
olm_filldir. In essence the locking was done at the lowest levels.
With newer kernels this low level locking preventing mounting
of the server back end with the following back trace:

 [  612.287442] mount.lustre    D    0  3026   3025 0x00000224
 [  612.289699] Call trace:
 [  612.290398] [<ffff000008085a8c>] __switch_to+0x8c/0xa8
 [  612.292363] [<ffff00000880b9d0>] __schedule+0x328/0x860
 [  612.294394] [<ffff00000880bf3c>] schedule+0x34/0x8c
 [  612.296285] [<ffff00000880f460>] rwsem_down_write_failed+0x134/0x238
 [  612.304803] [<ffff00000880e89c>] down_write+0x54/0x58
 [  612.307115] [<ffff0000028cae2c>] osd_ios_root_fill+0xd4/0x590 [osd_ldiskfs]
 [  612.310109] [<ffff000002281798>] call_filldir+0xd8/0x148 [ldiskfs]
 [  612.312450] [<ffff000002282170>] ldiskfs_readdir+0x670/0x7b8 [ldiskfs]
 [  612.314975] [<ffff0000082b18d0>] iterate_dir+0x150/0x1b8
 [  612.317118] [<ffff0000028c24ac>] osd_ios_general_scan+0x104/0x2b8 [osd_ldiskfs]
 [  612.320299] [<ffff0000028cb384>] osd_initial_OI_scrub+0x9c/0x13c0 [osd_ldiskfs]
 [  612.323047] [<ffff0000028cd53c>] osd_scrub_setup+0xb44/0x1118 [osd_ldiskfs]
 [  612.325758] [<ffff00000289d4ec>] osd_device_alloc+0x544/0x950 [osd_ldiskfs]
 [  612.328637] [<ffff000001e29dac>] class_setup+0x7bc/0xd20 [obdclass]
 [  612.331324] [<ffff000001e33a30>] class_process_config+0x1708/0x2e90 [obdclass]
 [  612.334259] [<ffff000001e3a368>] do_lcfg+0x2b0/0x6d8 [obdclass]
 [  612.336704] [<ffff000001e3f49c>] lustre_start_simple+0x154/0x3f8 [obdclass]
 [  612.339694] [<ffff000001e74ee0>] osd_start+0x500/0xa40 [obdclass]
 [  612.342277] [<ffff000001e80a84>] server_fill_super+0x1d4/0x1848 [obdclass]
 [  612.345078] [<ffff000001e437a4>] lustre_fill_super+0x62c/0xdb0 [obdclass]
 [  612.347655] [<ffff0000082a02e0>] mount_nodev+0x5c/0xbc
 [  612.349954] [<ffff000001e3adc4>] lustre_mount+0x4c/0x80 [obdclass]
 [  612.352263] [<ffff0000082a1324>] mount_fs+0x54/0x16c
 [  612.354159] [<ffff0000082bfb6c>] vfs_kern_mount+0x58/0x154
 [  612.356246] [<ffff0000082c2ff8>] do_mount+0x1cc/0xbac
 [  612.358192] [<ffff0000082c3d60>] SyS_mount+0x88/0xd4

This is due to ext4_readdir use of dir_relax_shared() that is taking the
inode_lock which conflicts with the lock taking in our dir_context actor.
Having the dir_context actor take the lock so deep in the stack is incorrect
behavior. Since this is the case we need to migrate the locking from the
dir_context actor to before ext4_readdir() is called. This ends up involving
implementing the locking in the code that calls fops->iterate_dir() osd-ldiskfs
implemented. Instead of handling the lock changes in at the osd-ldiskfs level
across many kernel versions we can use iterate_dir() directly which does this
handling for us.

To use interate_dir() we need to work around the fact that osd-ldiskfs does
not use the VFS layer to create or managed VFS data structure such as struct file
or struct dentry. Instead osd-ldiskfs uses these VFS data structures as scratch
areas just to interface with the ext4 layer. We could use the VFS layer to
manage these data structures but the would require a massive reworking of this
OSD abstraction. Instead we look at what pieces are missing from struct file
due to the open coding that iterate_dir() expects. The first thing missing
from struct file is the initialization of struct path which is used by
file_accessed() to update the atime. The current behavior for our OSD layer
is not to update the atime so we preserve this behavior by setting the
struct file f_flag field to O_NOATIME. This prevents the calling of
touch_atime() which expects a proper struct path. The second expected field
for iterate_dir() is f_security. The function security_file_alloc() is not
exported so we do this manually. For the Lustre OSD case we are not interested
in any file security messages since our object store is not exposed in any way.
We disable the sending of the fsnotify events by setting the f_mode flag to
FMODE_NONOTIFY.

With this migration of the lock handling up the OSD stack we can no longer
just use osd_lookup_one_len_unlocked(). Replace osd_lookup_one_len_unlocked()
with osd_lookup_one_len() for the cases when the inode_lock() is taken much
earlier in the stack. This also closely follows the behavior of similar
functions in the linux VFS layer.

Change-Id: I00893f41ef5ec01835f0e58da6bd5c96a62aea88
Signed-off-by: James Simmons <jsimmons@infradead.org>
Reviewed-on: https://review.whamcloud.com/34714
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Yang Sheng <ys@whamcloud.com>
Reviewed-by: Neil Brown <neilb@suse.de>
lustre/osd-ldiskfs/osd_compat.c
lustre/osd-ldiskfs/osd_handler.c
lustre/osd-ldiskfs/osd_internal.h
lustre/osd-ldiskfs/osd_oi.c
lustre/osd-ldiskfs/osd_scrub.c

index 15c395f..2150f28 100644 (file)
@@ -1298,7 +1298,6 @@ int osd_obj_map_recover(struct osd_thread_info *info,
        dquot_initialize(src_parent);
        dquot_initialize(dir);
 
-       inode_lock(src_parent);
        inode_lock(dir);
        bh = osd_ldiskfs_find_entry(dir, &tgt_child->d_name, &de, NULL, NULL);
        if (!IS_ERR(bh)) {
@@ -1323,7 +1322,6 @@ int osd_obj_map_recover(struct osd_thread_info *info,
                 */
                brelse(bh);
                inode_unlock(dir);
-               inode_unlock(src_parent);
                ldiskfs_journal_stop(jh);
 
                rc = -EEXIST;
@@ -1354,7 +1352,6 @@ int osd_obj_map_recover(struct osd_thread_info *info,
 
 unlock:
        inode_unlock(dir);
-       inode_unlock(src_parent);
        ldiskfs_journal_stop(jh);
        return rc;
 }
@@ -1431,7 +1428,8 @@ int osd_obj_spec_insert(struct osd_thread_info *info, struct osd_device *osd,
 }
 
 int osd_obj_spec_lookup(struct osd_thread_info *info, struct osd_device *osd,
-                       const struct lu_fid *fid, struct osd_inode_id *id)
+                       const struct lu_fid *fid, struct osd_inode_id *id,
+                       enum oi_check_flags flags)
 {
        struct dentry *root;
        struct dentry *dentry;
@@ -1456,7 +1454,8 @@ int osd_obj_spec_lookup(struct osd_thread_info *info, struct osd_device *osd,
                        RETURN(-ENOENT);
        }
 
-       dentry = osd_lookup_one_len_unlocked(osd, name, root, strlen(name));
+       dentry = osd_lookup_one_len_common(osd, name, root, strlen(name),
+                                          flags);
        if (!IS_ERR(dentry)) {
                inode = dentry->d_inode;
                if (inode) {
index 4905e5f..2edbbeb 100644 (file)
@@ -855,10 +855,8 @@ static int osd_check_lma(const struct lu_env *env, struct osd_object *obj)
 }
 
 struct osd_check_lmv_buf {
-#ifdef HAVE_DIR_CONTEXT
        /* please keep it as first member */
        struct dir_context ctx;
-#endif
        struct osd_thread_info *oclb_info;
        struct osd_device *oclb_dev;
        struct osd_idmap_cache *oclb_oic;
@@ -867,7 +865,7 @@ struct osd_check_lmv_buf {
 };
 
 /**
- * It is called internally by ->readdir() to filter out the
+ * It is called internally by ->iterate*() to filter out the
  * local slave object's FID of the striped directory.
  *
  * \retval     1 found the local slave's FID
@@ -964,9 +962,7 @@ static int osd_check_lmv(struct osd_thread_info *oti, struct osd_device *dev,
        const struct file_operations *fops;
        struct lmv_mds_md_v1 *lmv1;
        struct osd_check_lmv_buf oclb = {
-#ifdef HAVE_DIR_CONTEXT
                .ctx.actor = osd_stripe_dir_filldir,
-#endif
                .oclb_info = oti,
                .oclb_dev = dev,
                .oclb_oic = oic,
@@ -1013,21 +1009,19 @@ again:
        dentry->d_sb = inode->i_sb;
        filp->f_pos = 0;
        filp->f_path.dentry = dentry;
-       filp->f_mode = FMODE_64BITHASH;
+       filp->f_flags |= O_NOATIME;
+       filp->f_mode = FMODE_64BITHASH | FMODE_NONOTIFY;
        filp->f_mapping = inode->i_mapping;
        filp->f_op = fops;
        filp->private_data = NULL;
        set_file_inode(filp, inode);
+       rc = osd_security_file_alloc(filp);
+       if (rc)
+               goto out;
 
        do {
                oclb.oclb_items = 0;
-#ifdef HAVE_DIR_CONTEXT
-               oclb.ctx.pos = filp->f_pos;
-               rc = fops->iterate_shared(filp, &oclb.ctx);
-               filp->f_pos = oclb.ctx.pos;
-#else
-               rc = fops->readdir(filp, &oclb, osd_stripe_dir_filldir);
-#endif
+               rc = iterate_dir(filp, &oclb.ctx);
        } while (rc >= 0 && oclb.oclb_items > 0 && !oclb.oclb_found &&
                 filp->f_pos != LDISKFS_HTREE_EOF_64BIT);
        fops->release(inode, filp);
@@ -2434,6 +2428,14 @@ static int osd_commit_async(const struct lu_env *env,
 static int (*priv_dev_set_rdonly)(struct block_device *bdev);
 static int (*priv_dev_check_rdonly)(struct block_device *bdev);
 /* static int (*priv_dev_clear_rdonly)(struct block_device *bdev); */
+static int (*priv_security_file_alloc)(struct file *file);
+
+int osd_security_file_alloc(struct file *file)
+{
+       if (priv_security_file_alloc)
+               return priv_security_file_alloc(file);
+       return 0;
+}
 
 /*
  * Concurrency: shouldn't matter.
@@ -6515,13 +6517,11 @@ static void osd_it_ea_put(const struct lu_env *env, struct dt_it *di)
 }
 
 struct osd_filldir_cbs {
-#ifdef HAVE_DIR_CONTEXT
        struct dir_context ctx;
-#endif
        struct osd_it_ea  *it;
 };
 /**
- * It is called internally by ->readdir(). It fills the
+ * It is called internally by ->iterate*(). It fills the
  * iterator's in-memory data structure with required
  * information i.e. name, namelen, rec_size etc.
  *
@@ -6588,7 +6588,7 @@ static int osd_ldiskfs_filldir(void *buf,
 }
 
 /**
- * Calls ->readdir() to load a directory entry at a time
+ * Calls ->iterate*() to load a directory entry at a time
  * and stored it in iterator's in-memory data structure.
  *
  * \param di iterator's in memory structure
@@ -6607,9 +6607,7 @@ static int osd_ldiskfs_it_fill(const struct lu_env *env,
        struct file *filp = &it->oie_file;
        int rc = 0;
        struct osd_filldir_cbs buf = {
-#ifdef HAVE_DIR_CONTEXT
                .ctx.actor = osd_ldiskfs_filldir,
-#endif
                .it = it
        };
 
@@ -6625,13 +6623,15 @@ static int osd_ldiskfs_it_fill(const struct lu_env *env,
                down_read(&obj->oo_ext_idx_sem);
        }
 
-#ifdef HAVE_DIR_CONTEXT
-       buf.ctx.pos = filp->f_pos;
-       rc = inode->i_fop->iterate_shared(filp, &buf.ctx);
-       filp->f_pos = buf.ctx.pos;
-#else
-       rc = inode->i_fop->readdir(filp, &buf, osd_ldiskfs_filldir);
-#endif
+       rc = osd_security_file_alloc(filp);
+       if (rc)
+               RETURN(rc);
+
+       filp->f_flags |= O_NOATIME;
+       filp->f_mode |= FMODE_NONOTIFY;
+       rc = iterate_dir(filp, &buf.ctx);
+       if (rc)
+               RETURN(rc);
 
        if (hlock != NULL)
                ldiskfs_htree_unlock(hlock);
@@ -6655,7 +6655,7 @@ static int osd_ldiskfs_it_fill(const struct lu_env *env,
 }
 
 /**
- * It calls osd_ldiskfs_it_fill() which will use ->readdir()
+ * It calls osd_ldiskfs_it_fill() which will use ->iterate*()
  * to load a directory entry at a time and stored it in
  * iterator's in-memory data structure.
  *
@@ -7268,7 +7268,7 @@ static __u64 osd_it_ea_store(const struct lu_env *env, const struct dt_it *di)
 }
 
 /**
- * It calls osd_ldiskfs_it_fill() which will use ->readdir()
+ * It calls osd_ldiskfs_it_fill() which will use ->iterate*()
  * to load a directory entry at a time and stored it i inn,
  * in iterator's in-memory data structure.
  *
@@ -8196,6 +8196,8 @@ static int __init osd_init(void)
                return rc;
 
 #ifdef CONFIG_KALLSYMS
+       priv_security_file_alloc =
+               (void *)kallsyms_lookup_name("security_file_alloc");
        priv_dev_set_rdonly = (void *)kallsyms_lookup_name("dev_set_rdonly");
        priv_dev_check_rdonly =
                (void *)kallsyms_lookup_name("dev_check_rdonly");
index 8138b30..c3c62e5 100644 (file)
@@ -570,6 +570,8 @@ struct osd_iobuf {
        unsigned int       dr_init_at;  /* the line iobuf was initialized */
 };
 
+int osd_security_file_alloc(struct file *file);
+
 #ifdef HAVE_INODE_TIMESPEC64
 # define osd_timespec                  timespec64
 # define osd_timespec_trunc(ts, gran)  timespec64_trunc((ts), (gran))
@@ -788,7 +790,8 @@ int osd_obj_map_recover(struct osd_thread_info *info, struct osd_device *osd,
                        struct inode *src_parent, struct dentry *src_child,
                        const struct lu_fid *fid);
 int osd_obj_spec_lookup(struct osd_thread_info *info, struct osd_device *osd,
-                       const struct lu_fid *fid, struct osd_inode_id *id);
+                       const struct lu_fid *fid, struct osd_inode_id *id,
+                       enum oi_check_flags flags);
 int osd_obj_spec_insert(struct osd_thread_info *info, struct osd_device *osd,
                        const struct lu_fid *fid, const struct osd_inode_id *id,
                        handle_t *th);
index 44d56d9..1b132ad 100644 (file)
@@ -632,7 +632,7 @@ int osd_oi_lookup(struct osd_thread_info *info, struct osd_device *osd,
                  enum oi_check_flags flags)
 {
        if (unlikely(fid_is_last_id(fid)))
-               return osd_obj_spec_lookup(info, osd, fid, id);
+               return osd_obj_spec_lookup(info, osd, fid, id, flags);
 
        if (fid_is_llog(fid) || fid_is_on_ost(info, osd, fid, flags))
                return osd_obj_map_lookup(info, osd, fid, id);
@@ -651,7 +651,7 @@ int osd_oi_lookup(struct osd_thread_info *info, struct osd_device *osd,
                /* For other special FIDs, try OI first, then do spec lookup */
                rc = __osd_oi_lookup(info, osd, fid, id);
                if (rc == -ENOENT)
-                       return osd_obj_spec_lookup(info, osd, fid, id);
+                       return osd_obj_spec_lookup(info, osd, fid, id, flags);
                return rc;
        }
 
index f6d7e4a..144e335 100644 (file)
@@ -1728,10 +1728,8 @@ struct osd_ios_item {
 };
 
 struct osd_ios_filldir_buf {
-#ifdef HAVE_DIR_CONTEXT
        /* please keep it as first member */
        struct dir_context       ctx;
-#endif
        struct osd_thread_info  *oifb_info;
        struct osd_device       *oifb_dev;
        struct dentry           *oifb_dentry;
@@ -1956,7 +1954,8 @@ osd_ios_scan_one(struct osd_thread_info *info, struct osd_device *dev,
                                               inode);
        }
 
-       rc = osd_oi_lookup(info, dev, &tfid, id2, 0);
+       /* Since this called from iterate_dir() the inode lock will be taken */
+       rc = osd_oi_lookup(info, dev, &tfid, id2, OI_LOCKED);
        if (rc != 0) {
                if (rc != -ENOENT)
                        RETURN(rc);
@@ -2019,7 +2018,7 @@ static int osd_ios_lf_fill(void *buf,
                RETURN(0);
 
        scrub->os_lf_scanned++;
-       child = osd_lookup_one_len_unlocked(dev, name, parent, namelen);
+       child = osd_lookup_one_len(dev, name, parent, namelen);
        if (IS_ERR(child)) {
                CDEBUG(D_LFSCK, "%s: cannot lookup child '%.*s': rc = %d\n",
                      osd_name(dev), namelen, name, (int)PTR_ERR(child));
@@ -2093,8 +2092,7 @@ static int osd_ios_varfid_fill(void *buf,
        if (name[0] == '.')
                RETURN(0);
 
-       child = osd_lookup_one_len_unlocked(dev, name, fill_buf->oifb_dentry,
-                                           namelen);
+       child = osd_lookup_one_len(dev, name, fill_buf->oifb_dentry, namelen);
        if (IS_ERR(child))
                RETURN(PTR_ERR(child));
 
@@ -2142,8 +2140,7 @@ static int osd_ios_dl_fill(void *buf,
        if (map->olm_name == NULL)
                RETURN(0);
 
-       child = osd_lookup_one_len_unlocked(dev, name, fill_buf->oifb_dentry,
-                                           namelen);
+       child = osd_lookup_one_len(dev, name, fill_buf->oifb_dentry, namelen);
        if (IS_ERR(child))
                RETURN(PTR_ERR(child));
 
@@ -2177,8 +2174,7 @@ static int osd_ios_uld_fill(void *buf,
        if (name[0] != '[')
                RETURN(0);
 
-       child = osd_lookup_one_len_unlocked(dev, name, fill_buf->oifb_dentry,
-                                           namelen);
+       child = osd_lookup_one_len(dev, name, fill_buf->oifb_dentry, namelen);
        if (IS_ERR(child))
                RETURN(PTR_ERR(child));
 
@@ -2228,8 +2224,7 @@ static int osd_ios_root_fill(void *buf,
        if (map->olm_name == NULL)
                RETURN(0);
 
-       child = osd_lookup_one_len_unlocked(dev, name, fill_buf->oifb_dentry,
-                                           namelen);
+       child = osd_lookup_one_len(dev, name, fill_buf->oifb_dentry, namelen);
        if (IS_ERR(child))
                RETURN(PTR_ERR(child));
        else if (!child->d_inode)
@@ -2253,9 +2248,7 @@ osd_ios_general_scan(struct osd_thread_info *info, struct osd_device *dev,
                     struct dentry *dentry, filldir_t filldir)
 {
        struct osd_ios_filldir_buf    buf   = {
-#ifdef HAVE_DIR_CONTEXT
                                                .ctx.actor = filldir,
-#endif
                                                .oifb_info = info,
                                                .oifb_dev = dev,
                                                .oifb_dentry = dentry };
@@ -2269,21 +2262,19 @@ osd_ios_general_scan(struct osd_thread_info *info, struct osd_device *dev,
 
        filp->f_pos = 0;
        filp->f_path.dentry = dentry;
-       filp->f_mode = FMODE_64BITHASH;
+       filp->f_flags |= O_NOATIME;
+       filp->f_mode = FMODE_64BITHASH | FMODE_NONOTIFY;
        filp->f_mapping = inode->i_mapping;
        filp->f_op = fops;
        filp->private_data = NULL;
        set_file_inode(filp, inode);
+       rc = osd_security_file_alloc(filp);
+       if (rc)
+               RETURN(rc);
 
        do {
                buf.oifb_items = 0;
-#ifdef HAVE_DIR_CONTEXT
-               buf.ctx.pos = filp->f_pos;
-               rc = fops->iterate_shared(filp, &buf.ctx);
-               filp->f_pos = buf.ctx.pos;
-#else
-               rc = fops->readdir(filp, &buf, filldir);
-#endif
+               rc = iterate_dir(filp, &buf.ctx);
        } while (rc >= 0 && buf.oifb_items > 0 &&
                 filp->f_pos != LDISKFS_HTREE_EOF_64BIT);
        fops->release(inode, filp);
@@ -2347,10 +2338,12 @@ osd_ios_ROOT_scan(struct osd_thread_info *info, struct osd_device *dev,
         * to access the ".lustre" with cached IGIF. So we prefer
         * to the solution 2).
         */
+       inode_lock(dentry->d_inode);
        rc = osd_ios_scan_one(info, dev, dentry->d_inode,
                              child->d_inode, &LU_DOT_LUSTRE_FID,
                              dot_lustre_name,
                              strlen(dot_lustre_name), 0);
+       inode_unlock(dentry->d_inode);
        if (rc == -ENOENT) {
 out_scrub:
                /* It is 1.8 MDT device. */
@@ -2393,9 +2386,11 @@ osd_ios_OBJECTS_scan(struct osd_thread_info *info, struct osd_device *dev,
        if (IS_ERR(child)) {
                rc = PTR_ERR(child);
        } else {
+               inode_lock(dentry->d_inode);
                rc = osd_ios_scan_one(info, dev, dentry->d_inode,
                                      child->d_inode, NULL, ADMIN_USR,
                                      strlen(ADMIN_USR), 0);
+               inode_unlock(dentry->d_inode);
                dput(child);
        }
 
@@ -2407,9 +2402,11 @@ osd_ios_OBJECTS_scan(struct osd_thread_info *info, struct osd_device *dev,
        if (IS_ERR(child))
                GOTO(out, rc = PTR_ERR(child));
 
+       inode_lock(dentry->d_inode);
        rc = osd_ios_scan_one(info, dev, dentry->d_inode,
                              child->d_inode, NULL, ADMIN_GRP,
                              strlen(ADMIN_GRP), 0);
+       inode_unlock(dentry->d_inode);
        dput(child);
 out:
        RETURN(rc == -ENOENT ? 0 : rc);
@@ -2429,6 +2426,12 @@ static void osd_initial_OI_scrub(struct osd_thread_info *info,
        dev->od_igif_inoi = 1;
 
        while (1) {
+               /* Don't take inode_lock here since scandir() callbacks
+                * can call VFS functions which may manully take the
+                * inode lock itself like iterate_dir(). Since this
+                * is the case it is best to leave the scandir()
+                * callbacks to managing the inode lock.
+                */
                scandir(info, dev, dentry, filldir);
                if (item != NULL) {
                        dput(item->oii_dentry);