lkml.org 
[lkml]   [2010]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [rfc][patch] mm: lockdep page lock
    On Tue 16-03-10 13:21:53, Nick Piggin wrote:
    > On Mon, Mar 15, 2010 at 07:08:00PM +0100, Jan Kara wrote:
    > > Hi,
    > >
    > > On Tue 16-03-10 02:58:59, Nick Piggin wrote:
    > > > This patch isn't totally complete. Needs some nesting annotations for
    > > > filesystems like ntfs, and some async lock release annotations for other
    > > > end-io handlers, also page migration code needs to set the page lock
    > > > class. But the core of it is working nicely and is a pretty small patch.
    > > >
    > > > It is a bit different to one Peter posted a while back, with differences.
    > > > I don't care so much about bloating struct page with a few more bytes.
    > > > lockdep can't run on a production kernel so I think it's preferable to be
    > > > catching more complex errors than avoiding overhead. I also set the page
    > > > lock class at the time it is added to pagecache when we have the mapping
    > > > pinned to the page.
    > > >
    > > > One issue I wonder about is if the lock class is changed while some other
    > > > page locker is waiting to get the lock but has already called
    > > > lock_acquire for the old class. Possibly it could be solved if lockdep
    > > > has different primitives to say the caller is contending for a lock
    > > > versus if it has been granted the lock?
    > > >
    > > > Do you think it would be useful? --
    > > >
    > > > Page lock has very complex dependencies, so it would be really nice to
    > > > add lockdep support for it.
    > > >
    > > > For example: add_to_page_cache_locked(GFP_KERNEL) (called with page
    > > > locked) -> page reclaim performs a trylock_page -> page reclaim performs
    > > > a writepage -> writepage performs a get_block -> get_block reads
    > > > buffercache -> buffercache read requires grow_dev_page -> grow_dev_page
    > > > locks buffercache page -> if writepage fails, page reclaim calls
    > > > handle_write_error -> handle_write_error performs a lock_page
    > > >
    > > > So before even considering any other locks or more complex nested
    > > > filesystems, we can hold at least 3 different page locks at once. Should
    > > > be safe because we have an fs->bdev page lock ordering, and because
    > > > add_to_page_cache* tend to be called on new (non-LRU) pages that can't be
    > > > locked elsewhere, however a notable exception is tmpfs which moves live
    > > > pages in and out of pagecache.
    > > >
    > > > So lockdepify the page lock. Each filesystem type gets a unique key, to
    > > > handle inter-filesystem nesting (like regular filesystem -> buffercache,
    > > > or ecryptfs -> lower). Newly allocated pages get a default lock class,
    > > > and it is reassigned to their filesystem type when being added to page
    > > > cache.
    > > You'll probably soon notice that quite some filesystems (ext4, xfs,
    > > ocfs2, ...) lock several pages at once in their writepages function. The
    >
    > Yes indeed. This is what I had meant about nesting with NTFS, but I
    > understand that others do it too.
    >
    >
    > > locking rule here is that we always lock pages in index increasing order. I
    > > don't think lockdep will be able to handle something like that. Probably we
    > > can just avoid lockdep checking in these functions (or just acquire the
    > > page lock class for the first page) but definitely there will be some
    >
    > You are right, I don't think lockdep would work with that, so just
    > checking the lock for the first page should be better than nothing.
    > It might require some lockdep support in order to add context so it
    > doesn't go mad when unlock_page is called (would rather not add any
    > page flags to track that).
    >
    > If we were really clever and able to get back to the address of
    > struct page that _is_ holding the lock, we could just do a simple
    > check to ensure its index is < the index of the page we are trying
    > to take.
    >
    > That would give reasonable nesting checking without requiring lockdep
    > to track new chains for every page (obviously not feasible).
    This is an interesting idea. We could store a pointer to the
    first locked page (which is attached to some mapping) in task_struct.
    That should work fine.
    Honza
    --
    Jan Kara <jack@suse.cz>
    SUSE Labs, CR


    \
     
     \ /
      Last update: 2010-03-16 12:55    [W:0.034 / U:1.000 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site