Messages in this thread | | | Date | Tue, 30 May 2006 14:13:01 +1000 | From | Nick Piggin <> | Subject | Re: [rfc][patch] remove racy sync_page? |
| |
Andrew Morton wrote:
>On Tue, 30 May 2006 12:54:53 +1000 >Nick Piggin <nickpiggin@yahoo.com.au> wrote: > > >>I guess so. Is plugging still needed now that the IO layer should >>get larger requests? Disabling it might result in a small initial >>request (although even that may be good for pipelining)... >> > >Mysterious question, that. A few years ago I think Jens tried pulling unplugging >out, but some devices still want it (magneto-optical storage iirc). And I >think we did try removing it, and it caused hurt. >
Would be nice to get rid of it. I guess that's out of the question for several releases, even if there was no performance problems.
> >>Otherwise, we could make set_page_dirty_lock use a weird non-unplugging >>variant >> > >Yes, that would work. In fact the number of times when direct-io actually >calls set_page_dirty_lock() is infinitesimal - I had to jump through hoops >to even test that code. The speculative >set-the-page-dirty-before-we-do-the-IO thing is very effective. So the >performace impact of making such a change would be nil. >
OK, that'll probably be the way to go for upcoming releases. I'll post a patch soon.
> >That's for the direct-io case. Other cases might be hurt more. > >Also, perhaps we could poke kblockd to do it for us. >
Hard, I think, to get back to ->mapping. Well not hard if we just use a non-syncing lock_page, but in that case we don't need kblockd.
> >>sync_page wants to get either the current mapping, or a NULL one. >>The sync_page methods must then be able to handle running into a >>NULL mapping. >> >>With splice, the mapping can change, so you can have the wrong >>sync_page callback run against the page. >> > >Oh. >
Don't know what we can do about that off the top of my head. Within block_sync_page there shouldn't be any problems (at worst, we'd unplug the wrong dev). Maybe the whole sync_page callback scheme can be removed so nobody tries to do anything funny with it? Call blk_run_backing_dev directly.
> >>>>Well yes, writing to a page would be the main reason to set it dirty. >>>>Is splice broken as well? I'm not sure that it always has a ref on the >>>>inode when stealing a page. >>>> >>>> >>>Whereabouts? >>> >>> >>The ->pin() calls in pipe_to_file and pipe_to_sendpage? >> > >One for Jens... > > >>>>It sounds like you think fixing the set_page_dirty_lock callers wouldn't >>>>be too difficult? I wouldn't know (although the ptrace one should be >>>>able to be turned into a set_page_dirty, because we're holding mmap_sem). >>>> >>>> >>>No, I think it's damn impossible ;) >>> >>>get_user_pages() has gotten us a random pagecache page. How do we >>>non-racily get at the address_space prior to locking that page? >>> >>>I don't think we can. >>> >>> >>But the vma isn't going to disappear because mmap_sem is held; and the >>vma should hold a ref on the inode I think? >> > >That's true during the get_user_pages() call. Be we run >set_page_dirty_lock() much later, after IO completion. >
Oh, I thought you specifically meant the ptrace case. Yes, in general it is much harder.
>>Anyway, it is possible that most of the problems could be solved by locking >>the page at the time of lookup, and unlocking it on completion/dirtying... >>it's just that that would be a bit of a task. >> > >But lock_page() requires access to the address_space. To kick the IO so we >don't wait for ever. >
It shouldn't wait for ever, because the unplug timer will go off and kblockd will do it eventually. And I was imagining that it would have a pin on the address space at the point it is looked up... But reworking all callers is just a pipe dream anyway, so nevermind ;)
Where we have synchronous IO, we do want the queue to be unplugged, however in most set_page_dirty_lock case this is normally not the case and a locked page should be rare. So hmm yes that would make sense to have it use a special lock_page...
> >>Can we somehow add BUG_ONs to >>lock_page to ensure we've got an inode ref? >> > >WARN_ONs. >
But is it practical? Or I suspect the warning would only ever trigger in the really rare racy cases anyway, when dentry, inode, etc have been reclaimed.
Anyway, I'll come up with a less intrusive patch shortly...
--
Send instant messages to your online friends http://au.messenger.yahoo.com
- 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/
| |