lkml.org 
[lkml]   [2006]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 5/7] FS-Cache: Generic filesystem caching facility
    Date
    Andrew Morton <akpm@osdl.org> wrote:

    > Interesting. Perhaps a comment in there?

    Yep. It might even be worth making the actual check in linux/slab.h.

    > We already did an up_read().

    Yep. Also the initialisation was done on the wrong variable (tag not
    xtag)... which only worked because list_for_each_entry() always leaves a valid
    pointer in tag.

    > We have kmem_cache_zalloc() now.

    It's not in the NFS tree I've just pulled. Of course, git may be being a
    complete git again. I blame the original author:-)

    > This looks livelockable.

    How so? The bit at the top of the function makes sure that we can't get any
    new objects.

    > > + cache->ops->unlock_object(object);
    >
    > I assume ->lock_object() provides the locking for the hlist_add_head()?

    Half of it... Cookie lookup and withdrawal has to deal with the interaction
    between two locks:

    (1) The cookie lock. This is taken first when approached from the netfs side.

    (2) The object lock. This is taken first when approached from the cache side.

    The cookie lock is superior to the netfs lock, and fscache_withdraw_object()
    has to do lock reordering when withdrawing a cache from the system.

    fscache_lookup_object() - to which you refer - must be called with the cookie
    already write-locked.

    > > + down_write(&cookie->sem);
    > > + down_read(&cookie->parent->sem);
    >
    > That's a surprising locking order. Normally things are parent-first.

    But the operation starts from the child cookie.

    The order I've defined is that if both a cookie and its parent already exist,
    then you must get the lock on the child before the lock on the parent if you
    want both.

    Lookup is okay, since the new cookie doesn't exist to the rest of the system
    until we've finished, and should we need to walk up the index tree
    instantiating indices, the locking is in the right order to just do that.

    > > + /* prevent the cache from being withdrawn */
    > > + if (down_read_trylock(&object->cache->withdrawal_sem)) {
    >
    > trylock is often a sign that something is mucked up. A comment describing
    > why it is needed is always appropriate.

    In this case, it's not something mucked up. The only thing that write-locks
    cache->withdrawal_sem is the fscache_withdraw_cache(), and that then prevents
    any further operations on that cache succeeding.

    Obviously, before we withdraw the cache, down_read_trylock() will always
    succeed, and after we withdraw the cache it will always fail; but not only
    that, _whilst_ there's an operation in progress, the withdrawal will be made to
    wait because that does down_write() *not* down_write_trylock().

    I wonder if I should write some docs on the locking procedures used by
    FS-Cache.

    > That's your fourth implementation of kenter(). Maybe we
    > need <linux/dhowells.h>?

    :-)

    Maybe I should move my debugging macros into include/linux, but then everyone
    else would complain if their own versions weren't put in there, or would
    complain if they were forced to use mine.

    > u16 would be preferred.

    As I recall, Linus said something like that uint16_t and co are fine in one's
    own interfaces. I did actually ask him about that.

    u16 should die really, IMO.

    > > +int fscache_debug = 0;
    >
    > unneeded initialisation

    Yep.

    > > +static int fscache_init(void)
    >
    > __init?

    Yep.

    > Don't empty kobject release() functions upset Greg?

    I hope so... then he might improve his docs:-)

    The function would seem to be mandatory, but there really isn't anything for it
    to do, since the object is static:-/

    > Removeable?

    Yeah. It's handy to have around for debugging, but it's very arch specific,
    and can be added back later easily enough.

    > Large?

    Depends how you define "large".

    It doesn't actually produce very much code.

    > Defining symbols which are owned by the Kconfig system isn't very nice.

    Kconfig is still broken:

    warthog>grep -r CONFIG_FSCACHE include/linux/autoconf.h
    #define CONFIG_FSCACHE_MODULE 1
    warthog>

    Modules that might depend on fscache need to know that it's there, and having
    to double up every #if to detect both is stupid.

    Would you suggest then:

    #if defined(CONFIG_FSCACHE) || defined(CONFIG_FSCACHE_MODULE)
    #define FSCACHE_AVAILABLE 1
    #endif

    > hm. I tend to find it better for functions to be documented at the
    > definition site, not at the declaration site. I don't expect people even
    > think to go looking in a header file to learn about the function which
    > they're presently looking at.

    Two points:

    (1) The function they'll be looking for isn't in a .c file.

    (2) They *should* be looking in Documentation/ where all the kernel's
    interface functions are of course documented. They will find the
    documentation for the FS-Cache interfaces there. I'll add obvious banners
    to the tops of the header files telling them where to look. I can even
    add them to the .c files, I suppose.

    Now I admit that I haven't added docs for get_empty_filp(),
    dentry_open_kernel() and suchlike because there's nowhere to actually add
    them yet.

    David
    -
    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/

    \
     
     \ /
      Last update: 2006-04-21 16:18    [W:2.597 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site