[lkml]   [2009]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy
    On 27/01/09 20:13 -0800, Joel Becker wrote:
    > On Thu, Dec 18, 2008 at 07:00:18PM +0100, Louis Rilling wrote:
    > > configfs_depend_item() recursively locks all inodes mutex from configfs root to
    > > the target item, which makes lockdep unhappy. The purpose of this recursive
    > > locking is to ensure that the item tree can be safely parsed and that the target
    > > item, if found, is not about to leave.
    > >
    > > This patch reworks configfs_depend_item() locking using configfs_dirent_lock.
    > > Since configfs_dirent_lock protects all changes to the configfs_dirent tree, and
    > > protects tagging of items to be removed, this lock can be used instead of the
    > > inodes mutex lock chain.
    > > This needs that the check for dependents be done atomically with
    > >
    > > Now lockdep looks happy with configfs.
    > This looks almost, but not quite right.
    > In the create path, we do configfs_new_dirent() before we set
    > sd->s_type. But configfs_new_dirent() attaches sd->s_sibling. So, in
    > aonther thread, configfs_depend_prep() can traverse this s_sibling
    > without CONFIGFS_USET_CREATING being set. This turns out to be safe
    > because CONFIGFS_DIR is also not set - but boy I'd like a comment about
    > that.

    Definitely agreed. I should have written this comment instead of letting you
    notice this.

    > What if we're in mkdir(2) in one thread and another thread is
    > trying to pin the parent directory? That is, we are inside
    > configfs_mkdir(parent, new_dentry, mode). The other thread is doing
    > configfs_depend_item(subsys, parent). With this patch, the other thread
    > will not take parent->i_mutex. It will happily determine that
    > parent is part of the tree and bump its s_dependent with no locking. Is
    > this OK?

    Yes this is the expected impact. It is OK because
    1) under a same critical section of configfs_dirent_lock, depend_item()
    checks that CONFIGFS_USET_DROPPING is not set and bumps s_dependent;
    2) under a same critical section of configfs_dirent_lock, configfs_rmdir()
    checks the s_dependent count and tries to set CONFIGFS_USET_DROPPING.

    > If it is - isn't this patch good without any other reason? That
    > is, aside from the issues of lockdep, isn't it better for
    > configfs_depend_item() to never have to worry about the VFS locks other
    > than the configfs root?

    Yes, this patch may look like an improvement, independently from lockdep. I
    think that locking is simpler this way, and this also removes the need for
    configfs_depend_rollback(). Moreover this moves towards the management of
    configfs_dirents protected by configfs_dirent_lock only. In the end, it's up to
    you to judge if this is a good direction ;)



    Dr Louis Rilling Kerlabs
    Skype: louis.rilling Batiment Germanium
    Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes 35700 Rennes
    [unhandled content-type:application/pgp-signature]
     \ /
      Last update: 2009-01-28 11:35    [W:0.023 / U:48.064 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site