On Thu, 10 Mar 2011 04:26:31 -0800 (PST)
Chris Mason <chris.mason@xxxxxxxxxx> wrote:
> Excerpts from Steve French's message of 2011-03-09 17:13:06 -0500:
> > On Wed, Mar 9, 2011 at 3:58 PM, Chris Mason <chris.mason@xxxxxxxxxx> wrote:
> > > Excerpts from Dave Chinner's message of 2011-03-09 16:51:48 -0500:
> > >> On Wed, Mar 09, 2011 at 01:44:24PM -0600, Steve French wrote:
> > >> > Have alternative approaches, other than using wait_on_page_writeback,
> > >> > been considered for solving the stable page write problem in similar
> > >> > cases (since only about 1 out of 5 linux file systems uses this call
> > >> > today).
> > >>
> > >> I think that is incorrect. write_cache_pages() does:
> > >>
> > >> 929 lock_page(page);
> > >> .....
> > >> 950 if (PageWriteback(page)) {
> > >> 951 if (wbc->sync_mode != WB_SYNC_NONE)
> > >> 952 wait_on_page_writeback(page);
> > >> 953 else
> > >> 954 goto continue_unlock;
> > >> 955 }
> > >> 956
> > >> 957 BUG_ON(PageWriteback(page));
> > >> 958 if (!clear_page_dirty_for_io(page))
> > >> 959 goto continue_unlock;
> > >> 960
> > >> 961 trace_wbc_writepage(wbc, mapping->backing_dev_info);
> > >> 962 ret = (*writepage)(page, wbc, data);
> > >>
> > >> so every filesystem using the generic_writepages code already does
> > >> this check and wait before .writepage is called. Hence only the
> > >> filesystems that do not use generic_writepages() or
> > >> mpage_writepages() need a specific check, and that means most
> > >> filesystems are actually waiting on writeback pages correctly.
> > >
> > > But checking here just means we don't start writeback on a page that is
> > > writeback, which is a good idea but not really related to stable pages?
> > >
> > > stable pages means we don't let mmap'd pages or file_write muck around
> > > with the pages while they are in writeback, so we need to wait in
> > > file_write and page_mkwrite.
> >
> > Isn't the file_write case covered by the i_mutex as
> > Documentation/filesystems/Locking implies (for write_begin/write_end).
> >
>
> Does cifs take i_mutex before writepage? The disk based filesystems
> don't. So, i_mutex protects file_write from other procs jumping into
> file_write, but it doesn't protect writeback from file_write jumping in
> and changing the pages while they are being sent to storage (or over the
> wire).
>
> Basically the model needs to be:
>
> file_write:
> lock the page
> wait on page writeback
>
> < new writeback cannot start because of the page lock >
> copy_from_user
> unlock the page
>
> We also use page_mkwrite to get notified when userland wants to change
> some page it has given to mmap. That needs to wait on page writeback as
> well.
>
No, cifs doesn't take the i_mutex in writepage, but the page is locked.
cifs_write_begin calls grab_cache_page_write_begin, which returns a
locked page and it's not unlocked until cifs_write_end.
So I'm not sure I understand the potential race here. A normal
write_begin/end file write will block on the page lock, and the page is
locked during any writeback (either via writepage or writepages).
The only real "danger" is from processes that have the page mmapped as
they don't care about the page lock at all. A page_mkwrite routine that
does a wait_on_page_writeback should prevent that however.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
