| Message ID | 1479980796-26161-4-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e3fce68 |
| Headers | show |
On Thu, Nov 24, 2016 at 10:46:33AM +0100, Jan Kara wrote: > Currently each filesystem (possibly through generic_file_direct_write() > or iomap_dax_rw()) takes care of invalidating page tables and evicting Just some nits about the commit message: the DAX I/O path function is now called dax_iomap_rw(), and no filesystems still use generic_file_direct_write() for DAX so you can probably remove it from the changelog - up to you. Aside from that: Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > hole pages from the radix tree when write(2) to the file happens. This > invalidation is only necessary when there is some block allocation > resulting from write(2). Furthermore in current place the invalidation > is racy wrt page fault instantiating a hole page just after we have > invalidated it. > > So perform the page invalidation inside dax_do_io() where we can do it > only when really necessary and after blocks have been allocated so > nobody will be instantiating new hole pages anymore. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/dax.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index 4534f0e232e9..ddf77ef2ca18 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -984,6 +984,17 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > if (WARN_ON_ONCE(iomap->type != IOMAP_MAPPED)) > return -EIO; > > + /* > + * Write can allocate block for an area which has a hole page mapped > + * into page tables. We have to tear down these mappings so that data > + * written by write(2) is visible in mmap. > + */ > + if ((iomap->flags & IOMAP_F_NEW) && inode->i_mapping->nrpages) { > + invalidate_inode_pages2_range(inode->i_mapping, > + pos >> PAGE_SHIFT, > + (end - 1) >> PAGE_SHIFT); > + } > + > while (pos < end) { > unsigned offset = pos & (PAGE_SIZE - 1); > struct blk_dax_ctl dax = { 0 }; > @@ -1042,23 +1053,6 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter, > if (iov_iter_rw(iter) == WRITE) > flags |= IOMAP_WRITE; > > - /* > - * Yes, even DAX files can have page cache attached to them: A zeroed > - * page is inserted into the pagecache when we have to serve a write > - * fault on a hole. It should never be dirtied and can simply be > - * dropped from the pagecache once we get real data for the page. > - * > - * XXX: This is racy against mmap, and there's nothing we can do about > - * it. We'll eventually need to shift this down even further so that > - * we can check if we allocated blocks over a hole first. > - */ > - if (mapping->nrpages) { > - ret = invalidate_inode_pages2_range(mapping, > - pos >> PAGE_SHIFT, > - (pos + iov_iter_count(iter) - 1) >> PAGE_SHIFT); > - WARN_ON_ONCE(ret); > - } > - > while (iov_iter_count(iter)) { > ret = iomap_apply(inode, pos, iov_iter_count(iter), flags, ops, > iter, dax_iomap_actor); > -- > 2.6.6 >
On Tue 29-11-16 15:31:38, Ross Zwisler wrote: > On Thu, Nov 24, 2016 at 10:46:33AM +0100, Jan Kara wrote: > > Currently each filesystem (possibly through generic_file_direct_write() > > or iomap_dax_rw()) takes care of invalidating page tables and evicting > > Just some nits about the commit message: the DAX I/O path function is now > called dax_iomap_rw(), and no filesystems still use > generic_file_direct_write() for DAX so you can probably remove it from the > changelog - up to you. Yeah, good spotting. Many things have changed through rebases. I've fixed up the changelog to reflect current state. > Aside from that: > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> Thanks. Honza > > > hole pages from the radix tree when write(2) to the file happens. This > > invalidation is only necessary when there is some block allocation > > resulting from write(2). Furthermore in current place the invalidation > > is racy wrt page fault instantiating a hole page just after we have > > invalidated it. > > > > So perform the page invalidation inside dax_do_io() where we can do it > > only when really necessary and after blocks have been allocated so > > nobody will be instantiating new hole pages anymore. > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/dax.c | 28 +++++++++++----------------- > > 1 file changed, 11 insertions(+), 17 deletions(-) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 4534f0e232e9..ddf77ef2ca18 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -984,6 +984,17 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > > if (WARN_ON_ONCE(iomap->type != IOMAP_MAPPED)) > > return -EIO; > > > > + /* > > + * Write can allocate block for an area which has a hole page mapped > > + * into page tables. We have to tear down these mappings so that data > > + * written by write(2) is visible in mmap. > > + */ > > + if ((iomap->flags & IOMAP_F_NEW) && inode->i_mapping->nrpages) { > > + invalidate_inode_pages2_range(inode->i_mapping, > > + pos >> PAGE_SHIFT, > > + (end - 1) >> PAGE_SHIFT); > > + } > > + > > while (pos < end) { > > unsigned offset = pos & (PAGE_SIZE - 1); > > struct blk_dax_ctl dax = { 0 }; > > @@ -1042,23 +1053,6 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter, > > if (iov_iter_rw(iter) == WRITE) > > flags |= IOMAP_WRITE; > > > > - /* > > - * Yes, even DAX files can have page cache attached to them: A zeroed > > - * page is inserted into the pagecache when we have to serve a write > > - * fault on a hole. It should never be dirtied and can simply be > > - * dropped from the pagecache once we get real data for the page. > > - * > > - * XXX: This is racy against mmap, and there's nothing we can do about > > - * it. We'll eventually need to shift this down even further so that > > - * we can check if we allocated blocks over a hole first. > > - */ > > - if (mapping->nrpages) { > > - ret = invalidate_inode_pages2_range(mapping, > > - pos >> PAGE_SHIFT, > > - (pos + iov_iter_count(iter) - 1) >> PAGE_SHIFT); > > - WARN_ON_ONCE(ret); > > - } > > - > > while (iov_iter_count(iter)) { > > ret = iomap_apply(inode, pos, iov_iter_count(iter), flags, ops, > > iter, dax_iomap_actor); > > -- > > 2.6.6 > >
diff --git a/fs/dax.c b/fs/dax.c index 4534f0e232e9..ddf77ef2ca18 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -984,6 +984,17 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data, if (WARN_ON_ONCE(iomap->type != IOMAP_MAPPED)) return -EIO; + /* + * Write can allocate block for an area which has a hole page mapped + * into page tables. We have to tear down these mappings so that data + * written by write(2) is visible in mmap. + */ + if ((iomap->flags & IOMAP_F_NEW) && inode->i_mapping->nrpages) { + invalidate_inode_pages2_range(inode->i_mapping, + pos >> PAGE_SHIFT, + (end - 1) >> PAGE_SHIFT); + } + while (pos < end) { unsigned offset = pos & (PAGE_SIZE - 1); struct blk_dax_ctl dax = { 0 }; @@ -1042,23 +1053,6 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter, if (iov_iter_rw(iter) == WRITE) flags |= IOMAP_WRITE; - /* - * Yes, even DAX files can have page cache attached to them: A zeroed - * page is inserted into the pagecache when we have to serve a write - * fault on a hole. It should never be dirtied and can simply be - * dropped from the pagecache once we get real data for the page. - * - * XXX: This is racy against mmap, and there's nothing we can do about - * it. We'll eventually need to shift this down even further so that - * we can check if we allocated blocks over a hole first. - */ - if (mapping->nrpages) { - ret = invalidate_inode_pages2_range(mapping, - pos >> PAGE_SHIFT, - (pos + iov_iter_count(iter) - 1) >> PAGE_SHIFT); - WARN_ON_ONCE(ret); - } - while (iov_iter_count(iter)) { ret = iomap_apply(inode, pos, iov_iter_count(iter), flags, ops, iter, dax_iomap_actor);