lkml.org 
[lkml]   [2009]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/2] configfs: Silence lockdep on mkdir() and rmdir()
    On Thu, Dec 18, 2008 at 07:00:17PM +0100, Louis Rilling wrote:
    > >From inside configfs it is not possible to serialize those recursive
    > locking with a top-level one, because mkdir() and rmdir() are already
    > called with inodes locked by the VFS. So using some
    > mutex_lock_nest_lock() is not an option.
    >
    > I am proposing two solutions:
    > 1) one that wraps recursive mutex_lock()s with
    > lockdep_off()/lockdep_on().
    > 2) (as suggested earlier by Peter Zijlstra) one that puts the
    > i_mutexes recursively locked in different classes based on their
    > depth from the top-level config_group created. This
    > induces an arbitrary limit (MAX_LOCK_DEPTH - 2 == 46) on the
    > nesting of configfs default groups whenever lockdep is activated
    > but this limit looks reasonably high. Unfortunately, this also
    > isolates VFS operations on configfs default groups from the others
    > and thus lowers the chances to detect locking issues.
    >
    > Nobody likes solution 1), what I can understand.

    Me too :-P

    > This patch implements solution 2). However lockdep is still not happy with
    > configfs_depend_item(). Next patch reworks the locking of
    > configfs_depend_item() and finally makes lockdep happy.

    <snip>

    > #define CONFIGFS_ROOT 0x0001
    > diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
    > index 8e93341..f21be74 100644
    > --- a/fs/configfs/dir.c
    > +++ b/fs/configfs/dir.c
    > @@ -94,6 +94,9 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * pare
    > INIT_LIST_HEAD(&sd->s_links);
    > INIT_LIST_HEAD(&sd->s_children);
    > sd->s_element = element;
    > +#ifdef CONFIG_LOCKDEP
    > + sd->s_depth = -1;
    > +#endif
    > spin_lock(&configfs_dirent_lock);
    > if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
    > spin_unlock(&configfs_dirent_lock);
    > @@ -176,6 +179,17 @@ static int init_symlink(struct inode * inode)
    > return 0;
    > }
    >
    > +#ifdef CONFIG_LOCKDEP
    > +static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
    > + struct configfs_dirent *sd)
    > +{
    > + int parent_depth = parent_sd->s_depth;
    > +
    > + if (parent_depth >= 0)
    > + sd->s_depth = parent_depth + 1;
    > +}
    > +#endif
    > +
    > static int create_dir(struct config_item * k, struct dentry * p,
    > struct dentry * d)
    > {
    > @@ -187,6 +201,9 @@ static int create_dir(struct config_item * k, struct dentry * p,
    > error = configfs_make_dirent(p->d_fsdata, d, k, mode,
    > CONFIGFS_DIR | CONFIGFS_USET_CREATING);
    > if (!error) {
    > +#ifdef CONFIG_LOCKDEP
    > + configfs_set_dir_dirent_depth(p->d_fsdata, d->d_fsdata);
    > +#endif
    > error = configfs_create(d, mode, init_dir);
    > if (!error) {
    > inc_nlink(p->d_inode);

    Can you change this to provide non-lockdep versions of
    functions? We don't want "#ifdef CONFIG_LOCKDEP" everywhere. What we
    want is the code to call functions unconditionally, and the functions to
    do nothing if lockdep is not enabled. Like:

    #ifdef CONFIG_LOCKDEP
    static inline void configfs_init_dir_dirent_depth(dirent)
    {
    dirent->s_depth = -1;
    }

    static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
    struct configfs_dirent *sd)
    {
    int parent_depth = parent_sd->s_depth;

    if (parent_depth >= 0)
    sd->s_depth = parent_depth + 1;
    }
    #else
    static inline void configfs_init_dir_dirent_depth(dirent)
    {
    }

    static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
    struct configfs_dirent *sd)
    {
    }
    #endif

    This makes the callsites much nicer to read:

    @@ -94,6 +94,9 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * pare
    INIT_LIST_HEAD(&sd->s_links);
    INIT_LIST_HEAD(&sd->s_children);
    sd->s_element = element;
    + configfs_init_dir_dirent_depth(sd);
    spin_lock(&configfs_dirent_lock);
    if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
    spin_unlock(&configfs_dirent_lock);
    @@ -187,6 +201,7 @@ static int create_dir(struct config_item * k, struct dentry * p,
    error = configfs_make_dirent(p->d_fsdata, d, k, mode,
    CONFIGFS_DIR | CONFIGFS_USET_CREATING);
    if (!error) {
    + configfs_set_dir_dirent_depth(p->d_fsdata, d->d_fsdata);
    error = configfs_create(d, mode, init_dir);
    if (!error) {
    inc_nlink(p->d_inode);
    Otherwise, this patch seems pretty straightforward.

    Joel

    --

    Life's Little Instruction Book #30

    "Never buy a house without a fireplace."

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127


    \
     
     \ /
      Last update: 2009-01-28 05:01    [W:0.036 / U:60.568 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site