Messages in this thread | | | Subject | Re: [PATCH v2 2/4] mm: add file_fdatawait_range and file_write_and_wait | From | Steven Whitehouse <> | Date | Mon, 31 Jul 2017 12:32:31 +0100 |
| |
Hi,
On 31/07/17 12:27, Jeff Layton wrote: > On Thu, 2017-07-27 at 08:48 -0400, Jeff Layton wrote: >> On Thu, 2017-07-27 at 10:49 +0200, Jan Kara wrote: >>> On Wed 26-07-17 13:55:36, Jeff Layton wrote: >>>> +int file_write_and_wait(struct file *file) >>>> +{ >>>> + int err = 0, err2; >>>> + struct address_space *mapping = file->f_mapping; >>>> + >>>> + if ((!dax_mapping(mapping) && mapping->nrpages) || >>>> + (dax_mapping(mapping) && mapping->nrexceptional)) { >>>> + err = filemap_fdatawrite(mapping); >>>> + /* See comment of filemap_write_and_wait() */ >>>> + if (err != -EIO) { >>>> + loff_t i_size = i_size_read(mapping->host); >>>> + >>>> + if (i_size != 0) >>>> + __filemap_fdatawait_range(mapping, 0, >>>> + i_size - 1); >>>> + } >>>> + } >>> Err, what's the i_size check doing here? I'd just pass ~0 as the end of the >>> range and ignore i_size. It is much easier than trying to wrap your head >>> around possible races with file operations modifying i_size. >>> >>> Honza >> I'm basically emulating _exactly_ what filemap_write_and_wait does here, >> as I'm leery of making subtle behavior changes in the actual writeback >> behavior. For example: >> >> -----------------8<---------------- >> static inline int __filemap_fdatawrite(struct address_space *mapping, >> int sync_mode) >> { >> return __filemap_fdatawrite_range(mapping, 0, LLONG_MAX, sync_mode); >> } >> >> int filemap_fdatawrite(struct address_space *mapping) >> { >> return __filemap_fdatawrite(mapping, WB_SYNC_ALL); >> } >> EXPORT_SYMBOL(filemap_fdatawrite); >> -----------------8<---------------- >> >> ...which then sets up the wbc with the right ranges and sync mode and >> kicks off writepages. But then, it does the i_size_read to figure out >> what range it should wait on (with the shortcut for the size == 0 case). >> >> My assumption was that it was intentionally designed that way, but I'm >> guessing from your comments that it wasn't? If so, then we can turn >> file_write_and_wait a static inline wrapper around >> file_write_and_wait_range. > FWIW, I did a bit of archaeology in the linux-history tree and found > this patch from Marcelo in 2004. Is this optimization still helpful? If > not, then that does simplify the code a bit. > > -------------------8<-------------------- > > [PATCH] small wait_on_page_writeback_range() optimization > > filemap_fdatawait() calls wait_on_page_writeback_range() with -1 as "end" > parameter. This is not needed since we know the EOF from the inode. Use > that instead. > > Signed-off-by: Marcelo Tosatti <marcelo.tosatti@cyclades.com> > Signed-off-by: Andrew Morton <akpm@osdl.org> > Signed-off-by: Linus Torvalds <torvalds@osdl.org> > --- > mm/filemap.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/mm/filemap.c b/mm/filemap.c > index 78e18b7639b6..55fb7b4141e4 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -287,7 +287,13 @@ EXPORT_SYMBOL(sync_page_range); > */ > int filemap_fdatawait(struct address_space *mapping) > { > - return wait_on_page_writeback_range(mapping, 0, -1); > + loff_t i_size = i_size_read(mapping->host); > + > + if (i_size == 0) > + return 0; > + > + return wait_on_page_writeback_range(mapping, 0, > + (i_size - 1) >> PAGE_CACHE_SHIFT); > } > EXPORT_SYMBOL(filemap_fdatawait); >
Does this ever get called in cases where we would not hold fs locks? In that case we definitely don't want to be relying on i_size,
Steve.
| |