fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size - Kernel

This is a discussion on fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size - Kernel ; Andrew, this patch has been in my git tree for a while. It affects current mainline users (i.e., ecryptfs). Do you think it can go to mainline soon? If not, post-25? (I can remind you then). This patch has a ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

  1. fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    Andrew, this patch has been in my git tree for a while. It affects current
    mainline users (i.e., ecryptfs). Do you think it can go to mainline soon?
    If not, post-25? (I can remind you then). This patch has a few changes, and
    I can break it into several smaller ones if desired:

    1. remove the 3rd arg to fsstack_copy_attr_all. There are no users for it:
    ecryptfs never used the 3rd arg; unionfs stopped using it a long time
    ago. Halcrow ok'ed this patch some time ago.

    2. add necessary locking for 32-bit smp systems in fsstack_copy_inode_size
    (courtesy Hugh Dickins).

    3. minor commenting style changes, and addition of copyrights which were
    missing.

    Thanks,
    Erez.

    --cut--here----cut--here----cut--here----cut--here----cut--here--


    fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    Acked-by: Mike Halcrow
    Signed-off-by: Erez Zadok

    diff --git a/include/linux/fs_stack.h b/include/linux/fs_stack.h
    index bb516ce..6b52faf 100644
    --- a/include/linux/fs_stack.h
    +++ b/include/linux/fs_stack.h
    @@ -1,17 +1,28 @@
    +/*
    + * Copyright (c) 2006-2007 Erez Zadok
    + * Copyright (c) 2006-2007 Josef 'Jeff' Sipek
    + * Copyright (c) 2006-2007 Stony Brook University
    + * Copyright (c) 2006-2007 The Research Foundation of SUNY
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License version 2 as
    + * published by the Free Software Foundation.
    + */
    +
    #ifndef _LINUX_FS_STACK_H
    #define _LINUX_FS_STACK_H

    -/* This file defines generic functions used primarily by stackable
    +/*
    + * This file defines generic functions used primarily by stackable
    * filesystems; none of these functions require i_mutex to be held.
    */

    #include

    /* externs for fs/stack.c */
    -extern void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
    - int (*get_nlinks)(struct inode *));
    -
    -extern void fsstack_copy_inode_size(struct inode *dst, const struct inode *src);
    +extern void fsstack_copy_attr_all(struct inode *dest, const struct inode *src);
    +extern void fsstack_copy_inode_size(struct inode *dst,
    + const struct inode *src);

    /* inlines */
    static inline void fsstack_copy_attr_atime(struct inode *dest,
    diff --git a/fs/stack.c b/fs/stack.c
    index 67716f6..4336f2b 100644
    --- a/fs/stack.c
    +++ b/fs/stack.c
    @@ -1,24 +1,42 @@
    +/*
    + * Copyright (c) 2006-2007 Erez Zadok
    + * Copyright (c) 2006-2007 Josef 'Jeff' Sipek
    + * Copyright (c) 2006-2007 Stony Brook University
    + * Copyright (c) 2006-2007 The Research Foundation of SUNY
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License version 2 as
    + * published by the Free Software Foundation.
    + */
    +
    #include
    #include
    #include

    -/* does _NOT_ require i_mutex to be held.
    +/*
    + * does _NOT_ require i_mutex to be held.
    *
    * This function cannot be inlined since i_size_{read,write} is rather
    * heavy-weight on 32-bit systems
    */
    void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
    {
    - i_size_write(dst, i_size_read((struct inode *)src));
    +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
    + spin_lock(&dst->i_lock);
    +#endif
    + i_size_write(dst, i_size_read(src));
    dst->i_blocks = src->i_blocks;
    +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
    + spin_unlock(&dst->i_lock);
    +#endif
    }
    EXPORT_SYMBOL_GPL(fsstack_copy_inode_size);

    -/* copy all attributes; get_nlinks is optional way to override the i_nlink
    +/*
    + * copy all attributes; get_nlinks is optional way to override the i_nlink
    * copying
    */
    -void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
    - int (*get_nlinks)(struct inode *))
    +void fsstack_copy_attr_all(struct inode *dest, const struct inode *src)
    {
    dest->i_mode = src->i_mode;
    dest->i_uid = src->i_uid;
    @@ -29,14 +47,6 @@ void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
    dest->i_ctime = src->i_ctime;
    dest->i_blkbits = src->i_blkbits;
    dest->i_flags = src->i_flags;
    -
    - /*
    - * Update the nlinks AFTER updating the above fields, because the
    - * get_links callback may depend on them.
    - */
    - if (!get_nlinks)
    - dest->i_nlink = src->i_nlink;
    - else
    - dest->i_nlink = (*get_nlinks)(dest);
    + dest->i_nlink = src->i_nlink;
    }
    EXPORT_SYMBOL_GPL(fsstack_copy_attr_all);
    diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
    index 5e59658..4621f89 100644
    --- a/fs/ecryptfs/dentry.c
    +++ b/fs/ecryptfs/dentry.c
    @@ -62,7 +62,7 @@ static int ecryptfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
    struct inode *lower_inode =
    ecryptfs_inode_to_lower(dentry->d_inode);

    - fsstack_copy_attr_all(dentry->d_inode, lower_inode, NULL);
    + fsstack_copy_attr_all(dentry->d_inode, lower_inode);
    }
    out:
    return rc;
    diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
    index e238611..687819d 100644
    --- a/fs/ecryptfs/inode.c
    +++ b/fs/ecryptfs/inode.c
    @@ -575,9 +575,9 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry,
    lower_new_dir_dentry->d_inode, lower_new_dentry);
    if (rc)
    goto out_lock;
    - fsstack_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode, NULL);
    + fsstack_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode);
    if (new_dir != old_dir)
    - fsstack_copy_attr_all(old_dir, lower_old_dir_dentry->d_inode, NULL);
    + fsstack_copy_attr_all(old_dir, lower_old_dir_dentry->d_inode);
    out_lock:
    unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
    dput(lower_new_dentry->d_parent);
    @@ -910,7 +910,7 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia)

    rc = notify_change(lower_dentry, ia);
    out:
    - fsstack_copy_attr_all(inode, lower_inode, NULL);
    + fsstack_copy_attr_all(inode, lower_inode);
    return rc;
    }

    diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
    index d25ac95..7eac062 100644
    --- a/fs/ecryptfs/main.c
    +++ b/fs/ecryptfs/main.c
    @@ -211,7 +211,7 @@ int ecryptfs_interpose(struct dentry *lower_dentry, struct dentry *dentry,
    d_add(dentry, inode);
    else
    d_instantiate(dentry, inode);
    - fsstack_copy_attr_all(inode, lower_inode, NULL);
    + fsstack_copy_attr_all(inode, lower_inode);
    /* This size will be overwritten for real files w/ headers and
    * other metadata */
    fsstack_copy_inode_size(inode, lower_inode);
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    On Wed, 2 Apr 2008 21:49:11 -0400 Erez Zadok wrote:

    > Andrew, this patch has been in my git tree for a while. It affects current
    > mainline users (i.e., ecryptfs). Do you think it can go to mainline soon?
    > If not, post-25? (I can remind you then). This patch has a few changes, and
    > I can break it into several smaller ones if desired:
    >
    > 1. remove the 3rd arg to fsstack_copy_attr_all. There are no users for it:
    > ecryptfs never used the 3rd arg; unionfs stopped using it a long time
    > ago. Halcrow ok'ed this patch some time ago.
    >
    > 2. add necessary locking for 32-bit smp systems in fsstack_copy_inode_size
    > (courtesy Hugh Dickins).
    >
    > 3. minor commenting style changes, and addition of copyrights which were
    > missing.
    >
    > Thanks,
    > Erez.
    >
    > --cut--here----cut--here----cut--here----cut--here----cut--here--
    >
    >
    > fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size


    I'm already carrying this, via git-unionfs. It makes it a bit hard to merge.
    I guess resending it during the 2.6.26 merge window would suit.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    On Wed, Apr 02, 2008 at 09:49:11PM -0400, Erez Zadok wrote:
    ....
    > +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
    > + spin_lock(&dst->i_lock);
    > +#endif


    I think you need to check CONFIG_PREEMPT as well.

    Josef 'Jeff' Sipek.

    --
    The reasonable man adapts himself to the world; the unreasonable one
    persists in trying to adapt the world to himself. Therefore all progress
    depends on the unreasonable man.
    - George Bernard Shaw
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    In message <20080403182001.GB30189@josefsipek.net>, "Josef 'Jeff' Sipek" writes:
    > On Wed, Apr 02, 2008 at 09:49:11PM -0400, Erez Zadok wrote:
    > ...
    > > +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
    > > + spin_lock(&dst->i_lock);
    > > +#endif

    >
    > I think you need to check CONFIG_PREEMPT as well.
    >
    > Josef 'Jeff' Sipek.


    I'm not sure if it's needed in case of CONFIG_PREEMPT. Anyone? The code
    for i_size_write (below), and the comment at the top of the function,
    suggest that the spinlock is needed only to prevent the lots seqcount.

    /*
    * NOTE: unlike i_size_read(), i_size_write() does need locking around it
    * (normally i_mutex), otherwise on 32bit/SMP an update of i_size_seqcount
    * can be lost, resulting in subsequent i_size_read() calls spinning forever.
    */
    static inline void i_size_write(struct inode *inode, loff_t i_size)
    {
    #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
    write_seqcount_begin(&inode->i_size_seqcount);
    inode->i_size = i_size;
    write_seqcount_end(&inode->i_size_seqcount);
    #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPT)
    preempt_disable();
    inode->i_size = i_size;
    preempt_enable();
    #else
    inode->i_size = i_size;
    #endif
    }


    BTW, some time ago I reviewed all callers of i_size_write. I did so again
    just now, and the results were the same:

    - a LOT of callers of i_size_write don't take any lock
    - some take another spinlock in a different data structure
    - those that do take the spinlock, do so unconditionally
    - only unionfs and fs/stack.c wrap the spinlock in

    #if BITS_PER_LONG == 32 && defined(CONFIG_SMP)

    Erez.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    On Thu, 3 Apr 2008, Josef 'Jeff' Sipek wrote:
    > On Wed, Apr 02, 2008 at 09:49:11PM -0400, Erez Zadok wrote:
    > ...
    > > +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
    > > + spin_lock(&dst->i_lock);
    > > +#endif

    >
    > I think you need to check CONFIG_PREEMPT as well.


    Don't i_size_read() and i_size_write() handle that case adequately
    themselves with preempt_dis/enable?

    If you mean it'd be better to have the preempt_dis/enable bracketing
    around the i_size_read() along with the i_size_write(): well, could
    do, but if lower i_size is changing underneath us in that way,
    it can just as well change again a moment later, so no point.

    The bit that worries me, that prevents me from saying Acked-by,
    is that I'm unsure of the extent to which lower filesystems are
    relying on i_mutex or something else for their i_size serialization.
    Using the i_lock here plays well with unionfs internally, and fixes
    the hangs I used to see; but relying on one locking scheme here and
    another below is uncomfortable for me. But it is an improvement.

    Hugh
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    On Thu, 3 Apr 2008, Erez Zadok wrote:
    > In message <20080403182001.GB30189@josefsipek.net>, "Josef 'Jeff' Sipek" writes:
    > > I think you need to check CONFIG_PREEMPT as well.

    >
    > I'm not sure if it's needed in case of CONFIG_PREEMPT. Anyone? The code
    > for i_size_write (below), and the comment at the top of the function,
    > suggest that the spinlock is needed only to prevent the lots seqcount.


    Correct.

    > BTW, some time ago I reviewed all callers of i_size_write. I did so again
    > just now, and the results were the same:
    >
    > - a LOT of callers of i_size_write don't take any lock


    They mostly know that i_mutex is already held (as i_size_write comment
    mentions); but I believe that's up to the individual filesystem.

    > - some take another spinlock in a different data structure
    > - those that do take the spinlock, do so unconditionally
    > - only unionfs and fs/stack.c wrap the spinlock in
    >
    > #if BITS_PER_LONG == 32 && defined(CONFIG_SMP)


    I chose to follow the #ifdeffery of i_size_write(),
    but you could do it unconditionally if you prefer:
    just a little more overhead when it's not needed.

    As I've said elsewhere, I don't think the result can be entirely
    safe against concurrent changes in the lower filesystem, using
    different locking; but I don't know how resilient unionfs is
    expected to be against messing directly with lower at the same
    time as upper level.

    Hugh
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

    On Thu, Apr 03, 2008 at 08:26:54PM +0100, Hugh Dickins wrote:
    > On Thu, 3 Apr 2008, Erez Zadok wrote:
    > > In message <20080403182001.GB30189@josefsipek.net>, "Josef 'Jeff' Sipek" writes:
    > > > I think you need to check CONFIG_PREEMPT as well.

    > >
    > > I'm not sure if it's needed in case of CONFIG_PREEMPT. Anyone? The code
    > > for i_size_write (below), and the comment at the top of the function,
    > > suggest that the spinlock is needed only to prevent the lots seqcount.

    >
    > Correct.


    True...

    > > BTW, some time ago I reviewed all callers of i_size_write. I did so again
    > > just now, and the results were the same:
    > >
    > > - a LOT of callers of i_size_write don't take any lock

    >
    > They mostly know that i_mutex is already held (as i_size_write comment
    > mentions); but I believe that's up to the individual filesystem.


    Is this function (fsstack_copy_inode_size) used only in unlocked paths? If
    there's little chance that it'll ever need to be used in locked paths, then
    sure, why not have the lock right in the function.

    Josef 'Jeff' Sipek.

    --
    My public GPG key can be found at
    http://www.josefsipek.net/gpg/public-0xC7958FFE.txt
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread