Re: Fwd: Fw: [2.6.20.4] BUG: dentry xattrs still in use in shrink_dcache_for_umount() with reiserfs

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff Mahoney wrote:
Andrea Righi wrote:
Jeff Mahoney wrote:
I have the patchset that I mentioned, but I'm not proposing it for 2.6.21.
It's much too invasive to be introduced in an -rc7, but it does include
locking changes that I believe avoid this bug.

Vladimir was right in his analysis that sometimes get_xa_root() takes the
reference once and other times twice, but not for the right reasons. I save
a reference to the xattr dir to avoid a lookup later, but when there are multiple
getxattrs or listxattrs as the first xattr operation on the file system, we can end
up taking a second reference when we shouldn't. This is because those operations
are protected by read locks and the ->xattr_root pointer isn't protected by anything
else. A quick fix would be to just extend the protection of the priv root's i_mutex
around the assignment, and test first. The right fix involves a complete rework of
the locking, and I have code to do that, it's just too late to include it in 2.6.21.

I'd love to know what Andrea (and now Andi Kleen) are doing to hit this now. There
haven't been any changes in this code in a while, and the shrink_dcache_for_umount()
has been around since October. I'm unable to reproduce locally so far, so if Andrea
or Andi could see if this fixes it for them, I'd appreciate it.
Jeff, your patch doesn't resolve, but you hit the problem. Actually, I
think the xattr_root pointer must be protected also in get_xa_root()
using the same private root i_mutex.

I've tested the following patch and it resolves in my case. What do you
think about it? could it be a reasonable fix?

Your patch fixes the problem. That'll teach me to spin a patch up while I'm headed
out the door. I think a cleaner solution for now is just to refactor get_xa_root,
__get_xa_root, and create_xa_root into one function to simplify the lookup and the
locking. I think I'll add another step to my patch set that removes the caching of
xattr_root entirely or creates in on mount, as the priv root is created. The privroot
must be cached to avoid a deadlock on the fs-root directory's i_mutex, but xattr_root
was cached as an optimzation. If we need to take privroot->i_mutex every time, there's
no point in caching it since it will probably be in the dcache anyway.

-Jeff


Ignore this patch. I missed a spot.

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFGKiEHLPWxlyuTD7IRApRTAJ0WeJzkH7julSus9iqx+LYoAAlFOwCfahu0
4qN7BR/monh7ZcQWZ7Vmz3Y=
=UNNw
-----END PGP SIGNATURE-----
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [rfc][patch] Avoid taking global tasklist_lock for single threadedprocess at getrusage()
    ... I think now it is possible to improve this patch. ... Nice patches to avoid tasklist lock on thread traversal. ... * lock or the siglock. ...
    (Linux-Kernel)
  • Re: [RFC 0/2] new kfifo API
    ... and I disagree about the locking changes. ... Except for Andi's NMI driver, ... Changing the behaviour of an existing interface without changing ... but the first patch will break something. ...
    (Linux-Kernel)
  • [RFT][PATCH 2/2] sata_mv: convert to new EH (v5)
    ... This patch contains a small but key race fix in the interrupt handler, ... LOCKING: ... Inherited from caller. ... * @ap: ATA channel to manipulate ...
    (Linux-Kernel)
  • Re: Concurrent access to /dev/urandom
    ... the problem is that the locking was ... and not in the extract_entropy loop. ... I'm currently travelling so I can't easily test this patch, ... where applications would be calling them in tight loops on threaded ...
    (Linux-Kernel)
  • Re: [PATCH] mm/filemap.c: unconditionally call mark_page_accessed
    ... from being evicted from the page cache despite frequent access. ... My gut says the benefits of this patch outweigh the cost. ... case of a fixed, non-changing access state, but this seems hard since ... you would avoid most mark_page_accessedinvocations of subsequent same-page reads ...
    (Linux-Kernel)
Loading