On Thu, May 28, 2009 at 09:01:15AM +0200, Nick Piggin wrote:
> I think the block_dump output in __mark_inode_dirty is missing dentry locking.
> Surely the i_dentry list can change any time, so we may not even *get* a
> dentry there. If we do get one by chance, then it would appear to be able to
> go away or get renamed at any time...
>
> I've just gone the very-safe route and used the dcache helper to find this,
> and done the full refcounting/locking thing. Also just moved it to its
> own function to reduce clutter a bit.
>
> Is my thinking correct?
>
> Thanks,
> Nick
> ---
> fs/fs-writeback.c | 41 ++++++++++++++++++++++++-----------------
> 1 file changed, 24 insertions(+), 17 deletions(-)
>
> Index: linux-2.6/fs/fs-writeback.c
> ===================================================================
> --- linux-2.6.orig/fs/fs-writeback.c
> +++ linux-2.6/fs/fs-writeback.c
> @@ -64,6 +64,28 @@ static void writeback_release(struct bac
> clear_bit(BDI_pdflush, &bdi->state);
> }
>
> +static noinline void block_dump___mark_inode_dirty(struct inode *inode)
> +{
> + if (inode->i_ino || strcmp(inode->i_sb->s_id, "bdev")) {
> + struct dentry *dentry;
> + const char *name = "?";
> +
> + dentry = d_find_alias(inode);
> + if (dentry) {
> + spin_lock(&dentry->d_lock);
> + name = (const char *) dentry->d_name.name;
> + }
> + printk(KERN_DEBUG
> + "%s(%d): dirtied inode %lu (%s) on %s\n",
> + current->comm, task_pid_nr(current), inode->i_ino,
> + name, inode->i_sb->s_id);
> + if (dentry) {
> + spin_unlock(&dentry->d_lock);
> + dput(dentry);
> + }
> + }
> +}
> +
> /**
> * __mark_inode_dirty - internal function
> * @inode: inode to mark
> @@ -114,23 +136,8 @@ void __mark_inode_dirty(struct inode *in
> if ((inode->i_state & flags) == flags)
> return;
>
> - if (unlikely(block_dump)) {
> - struct dentry *dentry = NULL;
> - const char *name = "?";
> -
> - if (!list_empty(&inode->i_dentry)) {
> - dentry = list_entry(inode->i_dentry.next,
> - struct dentry, d_alias);
> - if (dentry && dentry->d_name.name)
> - name = (const char *) dentry->d_name.name;
> - }
> -
> - if (inode->i_ino || strcmp(inode->i_sb->s_id, "bdev"))
> - printk(KERN_DEBUG
> - "%s(%d): dirtied inode %lu (%s) on %s\n",
> - current->comm, task_pid_nr(current), inode->i_ino,
> - name, inode->i_sb->s_id);
> - }
> + if (unlikely(block_dump))
> + block_dump___mark_inode_dirty(inode);
>
> spin_lock(&inode_lock);
> if ((inode->i_state & flags) != flags) {
It looks correct to me, you need to hold the dcache_lock to access the i_dentry
list safely, which you accomplish with d_find_alias, and you need to hold the
d_lock in order to safely access d_name, so it all looks right. Thanks,
Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
