lkml.org 
[lkml]   [2021]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6 5/7] kernfs: use i_lock to protect concurrent inode updates
    On Wed, Jun 09, 2021 at 04:51:22PM +0800, Ian Kent wrote:
    > The inode operations .permission() and .getattr() use the kernfs node
    > write lock but all that's needed is to keep the rb tree stable while
    > updating the inode attributes as well as protecting the update itself
    > against concurrent changes.

    Huh? Where does it access the rbtree at all? Confused...

    > diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
    > index 3b01e9e61f14e..6728ecd81eb37 100644
    > --- a/fs/kernfs/inode.c
    > +++ b/fs/kernfs/inode.c
    > @@ -172,6 +172,7 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode)
    > {
    > struct kernfs_iattrs *attrs = kn->iattr;
    >
    > + spin_lock(&inode->i_lock);
    > inode->i_mode = kn->mode;
    > if (attrs)
    > /*
    > @@ -182,6 +183,7 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode)
    >
    > if (kernfs_type(kn) == KERNFS_DIR)
    > set_nlink(inode, kn->dir.subdirs + 2);
    > + spin_unlock(&inode->i_lock);
    > }

    Even more so - just what are you serializing here? That code synchronizes inode
    metadata with those in kernfs_node. Suppose you've got two threads doing
    ->permission(); the first one gets through kernfs_refresh_inode() and goes into
    generic_permission(). No locks are held, so kernfs_refresh_inode() from another
    thread can run in parallel with generic_permission().

    If that's not a problem, why two kernfs_refresh_inode() done in parallel would
    be a problem?

    Thread 1:
    permission
    done refresh, all locks released now
    Thread 2:
    change metadata in kernfs_node
    Thread 2:
    permission
    goes into refresh, copying metadata into inode
    Thread 1:
    generic_permission()
    No locks in common between the last two operations, so
    we generic_permission() might see partially updated metadata.
    Either we don't give a fuck (in which case I don't understand
    what purpose does that ->i_lock serve) *or* we need the exclusion
    to cover a wider area.

    \
     
     \ /
      Last update: 2021-06-12 03:47    [W:4.541 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site