Messages in this thread | | | From | Neil Brown <> | Date | Wed, 31 May 2006 14:34:38 +1000 | Subject | Re: [rfc][patch] remove racy sync_page? |
| |
On Tuesday May 30, nickpiggin@yahoo.com.au wrote: > Neil Brown wrote: > > On Tuesday May 30, nickpiggin@yahoo.com.au wrote: > > > > As for your original problem.... I wonder if PG_locked is protecting > > too much? It protects against IO and it also protects against ->mapping > > changes. So if you want to ensure that ->mapping won't change, you > > need to wait for any pending read request to finish, which seems a bit > > dumb. > > I don't think that is the problem. set_page_dirty_lock is really > unlikely to get held up on read IO: that'd mean there were two things > writing into that page at the same time.
That's exactly my point - though I expand a bit more further down. i.e. set_page_dirty_lock is unlikely to get held up on a read IO, however it has to call into lock_page, and lock_page has no idea that it cannot be waiting for IO, and so it has to call sync_page just in case.
> > > > > Maybe we need a new bit: PG_maplocked. You are only allowed to change > > ->mapping or ->index of you hold PG_locked and PG_maplocked, you are > > not allowed to wait for PG_locked while holding PG_maplocked, and > > you can read ->mapping or ->index while PG_locked or PG_maplocked are > > held. > > Think of PG_locked like a mutex and PG_maplocked like a spinlock (and > > probably use bit_spinlock to get it). > > Well the original problem is fixed by not doing the sync_page thing in > set_page_dirty_lock. Is there any advantage to having another bit? > Considering a) it will be very unlikely that a page is locked at the > same time one would like to dirty it; and b) that would seem to imply > adding extra atomic ops and barriers to reclaim and truncate (maybe > others).
While I agree that avoiding the sync_page in this particular case is likely to solve the problem, it seems rather fragile. Sometimes you need to call ->sync_page when waiting for a lock, sometimes you don't. Why the difference? Because there really are two different locks here masquerading as one.
Yes there would be extra atomic ops at reclaim and truncate. Is that a problem? I have no idea, but you sometimes need to break eggs to fix an omelette. I suspect the extra bit would be very lightly contended.
> > > > > Then set_page_dirty_lock would use PG_maplocked to get access to > > ->mapping, and then hold a reference on the address_space while > > calling into balance_dirty_pages ... I wonder how you hold a reference > > on an address space... > > inode. Presumably PG_maplocked would pin it? I don't understand > why you've brought balance_dirty_pages into it, though. >
hmmmm.... no idea, sorry. I was trying to trace through set_page_dirty to see if it was safe to call it under a spinlock (or a bit_spinlock in this case). I must have taken a wrong turn somewhere...
> > > > There are presumably few pieces of code that change ->mapping. Once > > they all take PG_maplocked as well as PG_locked, you can start freeing > > up other code to take PG_maplocked instead of PG_locked.... > > > > Does that make sense at all? Do we have any spare page bits? > > I'm sure it could be made to work, but I don't really see the point. > If someone really wanted to do it, I guess the right way to go is have > a PG_readin counterpart to PG_writeback (or even extend PG_writeback > to PG_io)...
Yes, PG_readin or PG_io would be better names, but might be hard to have a graceful transition to that sort of naming.
And the point is that once you separated that function from PG_locked, lock_page would not need to wait for IO, and so would not need to call sync_page, and so your problem would evaporate.
NeilBrown - 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/
| |