Re: [Cluster-devel] [PATCH v4 09/11] gfs2: iomap direct I/O support

[Andreas Gruenbacher]

On 15 May 2018 at 09:31, Christoph Hellwig  wrote:
> On Mon, May 14, 2018 at 05:36:22PM +0200, Andreas Gruenbacher wrote:
>> With that, the direct_IO address space operation can be all but
>> eliminated: only a dummy remains which indicates that the filesystem
>> supports direct I/O.
>
> And that dummy can be replaced with the generic noop_direct_IO helper.

Ok, switching to that.

>> + ret = filemap_write_and_wait_range(mapping, lstart, end);
>> + if (ret)
>> + goto out;
>
> We already do this call in the common code.
>
>> + if (iov_iter_rw(from) == WRITE)
>> + truncate_inode_pages_range(mapping, lstart, end);
>
> Why do you need the truncate_inode_pages_range call here instead of
> invalidate_inode_pages2_range that everyone else uses and that
> the iomap code already does for you?

Indeed, I guess this whole code block can now be discarded. Taking
Steve into the CC since it's him who originally added that.

Thanks,
Andreas

Re: [Cluster-devel] [PATCH v4 09/11] gfs2: iomap direct I/O support

[Christoph Hellwig]

On Mon, May 14, 2018 at 05:36:22PM +0200, Andreas Gruenbacher wrote:
> With that, the direct_IO address space operation can be all but
> eliminated: only a dummy remains which indicates that the filesystem
> supports direct I/O.

And that dummy can be replaced with the generic noop_direct_IO helper.

> + ret = filemap_write_and_wait_range(mapping, lstart, end);
> + if (ret)
> + goto out;

We already do this call in the common code.

> + if (iov_iter_rw(from) == WRITE)
> + truncate_inode_pages_range(mapping, lstart, end);

Why do you need the truncate_inode_pages_range call here instead of
invalidate_inode_pages2_range that everyone else uses and that
the iomap code already does for you?

[Cluster-devel] [PATCH v4 09/11] gfs2: iomap direct I/O support

[Andreas Gruenbacher]

With that, the direct_IO address space operation can be all but
eliminated: only a dummy remains which indicates that the filesystem
supports direct I/O.

Signed-off-by: Andreas Gruenbacher 
---
 fs/gfs2/aops.c |  92 +--
 fs/gfs2/bmap.c |  11 +++-
 fs/gfs2/file.c | 169 ++---
 3 files changed, 170 insertions(+), 102 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 3d9633175aa8..d676ee63ab2b 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -84,12 +84,6 @@ static int gfs2_get_block_noalloc(struct inode *inode, 
sector_t lblock,
return 0;
 }
 
-static int gfs2_get_block_direct(struct inode *inode, sector_t lblock,
-struct buffer_head *bh_result, int create)
-{
-   return gfs2_block_map(inode, lblock, bh_result, 0);
-}
-
 /**
  * gfs2_writepage_common - Common bits of writepage
  * @page: The page to be written
@@ -1021,94 +1015,12 @@ static void gfs2_invalidatepage(struct page *page, 
unsigned int offset,
try_to_release_page(page, 0);
 }
 
-/**
- * gfs2_ok_for_dio - check that dio is valid on this file
- * @ip: The inode
- * @offset: The offset at which we are reading or writing
- *
- * Returns: 0 (to ignore the i/o request and thus fall back to buffered i/o)
- *  1 (to accept the i/o request)
- */
-static int gfs2_ok_for_dio(struct gfs2_inode *ip, loff_t offset)
-{
-   /*
-* Should we return an error here? I can't see that O_DIRECT for
-* a stuffed file makes any sense. For now we'll silently fall
-* back to buffered I/O
-*/
-   if (gfs2_is_stuffed(ip))
-   return 0;
-
-   if (offset >= i_size_read(>i_inode))
-   return 0;
-   return 1;
-}
-
-
-
 static ssize_t gfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
-   struct file *file = iocb->ki_filp;
-   struct inode *inode = file->f_mapping->host;
-   struct address_space *mapping = inode->i_mapping;
-   struct gfs2_inode *ip = GFS2_I(inode);
-   loff_t offset = iocb->ki_pos;
-   struct gfs2_holder gh;
-   int rv;
-
/*
-* Deferred lock, even if its a write, since we do no allocation
-* on this path. All we need change is atime, and this lock mode
-* ensures that other nodes have flushed their buffered read caches
-* (i.e. their page cache entries for this inode). We do not,
-* unfortunately have the option of only flushing a range like
-* the VFS does.
+* We just need the method present so that open/fcntl allow direct I/O.
 */
-   gfs2_holder_init(ip->i_gl, LM_ST_DEFERRED, 0, );
-   rv = gfs2_glock_nq();
-   if (rv)
-   goto out_uninit;
-   rv = gfs2_ok_for_dio(ip, offset);
-   if (rv != 1)
-   goto out; /* dio not valid, fall back to buffered i/o */
-
-   /*
-* Now since we are holding a deferred (CW) lock at this point, you
-* might be wondering why this is ever needed. There is a case however
-* where we've granted a deferred local lock against a cached exclusive
-* glock. That is ok provided all granted local locks are deferred, but
-* it also means that it is possible to encounter pages which are
-* cached and possibly also mapped. So here we check for that and sort
-* them out ahead of the dio. The glock state machine will take care of
-* everything else.
-*
-* If in fact the cached glock state (gl->gl_state) is deferred (CW) in
-* the first place, mapping->nr_pages will always be zero.
-*/
-   if (mapping->nrpages) {
-   loff_t lstart = offset & ~(PAGE_SIZE - 1);
-   loff_t len = iov_iter_count(iter);
-   loff_t end = PAGE_ALIGN(offset + len) - 1;
-
-   rv = 0;
-   if (len == 0)
-   goto out;
-   if (test_and_clear_bit(GIF_SW_PAGED, >i_flags))
-   unmap_shared_mapping_range(ip->i_inode.i_mapping, 
offset, len);
-   rv = filemap_write_and_wait_range(mapping, lstart, end);
-   if (rv)
-   goto out;
-   if (iov_iter_rw(iter) == WRITE)
-   truncate_inode_pages_range(mapping, lstart, end);
-   }
-
-   rv = __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev, iter,
- gfs2_get_block_direct, NULL, NULL, 0);
-out:
-   gfs2_glock_dq();
-out_uninit:
-   gfs2_holder_uninit();
-   return rv;
+   return -EINVAL;
 }
 
 /**
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 7ac08660c990..6f57e2bfab8e 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1053,7 +1053,14 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, 
loff_t length,
 
trace_gfs2_iomap_start(ip, pos, length, flags);
if (flags & IOMAP_WRITE)