xfs
[Top] [All Lists]

Re: [PATCH 1/5] [XFS] Use xfs_sync_inodes() for device flushing

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/5] [XFS] Use xfs_sync_inodes() for device flushing
From: Mikulas Patocka <mpatocka@xxxxxxxxxx>
Date: Wed, 25 Mar 2009 11:19:47 -0400 (EDT)
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20090320043027.GO26138@disturbed>
References: <1237114679-18808-1-git-send-email-david@xxxxxxxxxxxxx> <1237114679-18808-2-git-send-email-david@xxxxxxxxxxxxx> <Pine.LNX.4.64.0903170857050.26754@xxxxxxxxxxxxxxxxxxxxxxxxxxx> <20090318041752.GN26138@disturbed> <Pine.LNX.4.64.0903181207390.19233@xxxxxxxxxxxxxxxxxxxxxxxxxxx> <20090320043027.GO26138@disturbed>

On Fri, 20 Mar 2009, Dave Chinner wrote:

> On Wed, Mar 18, 2009 at 12:14:51PM -0400, Mikulas Patocka wrote:
> > On Wed, 18 Mar 2009, Dave Chinner wrote:
> > > On Tue, Mar 17, 2009 at 09:08:36AM -0400, Mikulas Patocka wrote:
> > > > On Sun, 15 Mar 2009, Dave Chinner wrote:
> > What I find scary is those two recursive pagelocks being held without 
> > trylock. The sequence is like:
> > 
> > lock iolock 1
> > lock pagelock 1
> > --- submit request to xfssyncd, that does
> > trylock iolock 2
> > lock pagelock 2
> 
> Those two pages will be on different inodes, so locking through all
> paths to pagelock 2 except for page writeback will be protected by the 
> iolocks...
> 
> > ... now suppose that this is racing with another process that takes 
> > pagelock without taking iolock first (memory manager trying to flush files 
> > mmaped for write or so). It can do
> > 
> > lock pagelock 2
> > ... and submit flush request to a thread that is actually waiting to get 
> > pagelock 2.
> 
> Which, AFAIK, is never done in XFS. Once we have a page locked in
> the writeback path we either dispatch it in an IO or unlock it
> directly before continuing. There should not be a case like you
> describe occurring (it is a bug if that does happen).
> 
> > --- I don't know --- is there a possibility to reserve filesystem space 
> > for write-mapped files at the time of the first page fault? (so that the 
> > space won't be allocated at the time of a flush?).
> 
> ->page_mkwrite

This is called without page lock so it is ok.

So I think there is no deadlock possibility.

But I still say that it is scary to take two pagelocks recursively and 
there is risk that someone will forget about these specific requirements 
after few years and make a bug.

Mikulas

<Prev in Thread] Current Thread [Next in Thread>