Re: [Cluster-devel] [PATCH v2 0/6] SEEK_HOLE / SEEK_DATA via iomap

[Andreas Gruenbacher]

On Mon, Jun 26, 2017 at 11:50 AM, Christoph Hellwig  wrote:
> On Fri, Jun 23, 2017 at 03:34:38PM +0200, Andreas Gruenbacher wrote:
>> These patches, on top of the page_cache_seek_hole_data patches posted earlier
>> today, convert xfs to implement SEEK_HOLE / SEEK_DATA via iomap, and 
>> implement
>> SEEK_HOLE / SEEK_DATA via iomap in gfs2.
>
> Please send those as one series - in fact to me the previous
> series doesn't even make sense - just introduce page_cache_seek_hole_data
> for the iomap code only as everyone should be using that.
>
>> ext4 doesn't implement IOMAP_REPORT, so it can't be switched to use the iomap
>> based SEEK_HOLE / SEEK_DATA or fiemap code so far.
>
> Just treat it as a no-op, it only really matters for the reflink case.

I'm actually not sure what you're talking about. The
page_cache_seek_hole_data patches fix a bug in the lseek
implementations of xfs and ext4. We want those bugs to be fixed on
both filesystems. On ext4, we cannot switch to iomap_seek_hole_data
because ext4 doesn't implement IOMAP_REPORT. We also cannot fall back
to a dummy IOMAP_REPORT function that reports the whole file as data
because that would be a regression -- after all, ext4 supports lseek
SEEK_HOLE / SEEK_DATA already.

We could squash the two xfs patches together, but __xfs_seek_hole_data
can only be removed once your patch for cleaning up the quota code is
merged, so the combined patch would convert __xfs_seek_hole_data to
use page_cache_seek_hole_data and xfs_seek_hole_data to use
iomap_seek_hole_data at the same time, which is a bit messy.

Thanks,
Andreas

Re: [Cluster-devel] [PATCH v2 1/6] vfs: Add iomap_seek_hole_data helper

[Andreas Gruenbacher]

On Mon, Jun 26, 2017 at 12:47 PM, Christoph Hellwig  wrote:
> On Fri, Jun 23, 2017 at 03:34:39PM +0200, Andreas Gruenbacher wrote:
>> Filesystems can use this for implementing lseek SEEK_HOLE / SEEK_DATA
>> support via iomap.
>>
>> Signed-off-by: Andreas Gruenbacher 
>> ---
>>  fs/iomap.c| 89 
>> +++
>>  include/linux/iomap.h |  3 ++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/fs/iomap.c b/fs/iomap.c
>> index 4b10892..781f0a0 100644
>> --- a/fs/iomap.c
>> +++ b/fs/iomap.c
>> @@ -584,6 +584,95 @@ int iomap_fiemap(struct inode *inode, struct 
>> fiemap_extent_info *fi,
>>  }
>>  EXPORT_SYMBOL_GPL(iomap_fiemap);
>>
>> +static loff_t
>> +iomap_seek_hole_actor(struct inode *inode, loff_t offset, loff_t length,
>> +   void *data, struct iomap *iomap)
>> +{
>> + if (iomap->type == IOMAP_HOLE)
>> + goto found;
>> + length = iomap->offset + iomap->length - offset;
>
> What is the problem with the passed in length value?

Yep, no need to recompute that.

>> + if (iomap->type != IOMAP_UNWRITTEN)
>> + return length;
>> + offset = page_cache_seek_hole_data(inode, offset, length, SEEK_HOLE);
>> + if (offset < 0)
>> + return length;
>> +found:
>> + *(loff_t *)data = offset;
>> + return 0;
>
> What about using a switch statement?
>
> switch (iomap->type) {
> case IOMAP_UNWRITTEN:
> offset = page_cache_seek_hole_data(inode, offset, length,
> SEEK_HOLE);
> if (offset < 0)
> return length;
> /*FALLTHRU*/
> case IOMAP_HOLE:
> *(loff_t *)data = offset;
> return 0;
> default:
> return length;
> }

Ok.

>> +static loff_t
>> +iomap_seek_data_actor(struct inode *inode, loff_t offset, loff_t length,
>> +   void *data, struct iomap *iomap)
>> +{
>> + if (iomap->type != IOMAP_HOLE && iomap->type != IOMAP_UNWRITTEN)
>> + goto found;
>> + length = iomap->offset + iomap->length - offset;
>> + if (iomap->type != IOMAP_UNWRITTEN)
>> + return length;
>> + offset = page_cache_seek_hole_data(inode, offset, length, SEEK_DATA);
>> + if (offset < 0)
>> + return length;
>> +found:
>> + *(loff_t *)data = offset;
>> + return 0;
>
>> +loff_t
>> +iomap_seek_hole_data(struct inode *inode, loff_t offset,
>> +  int whence, const struct iomap_ops *ops)
>> +{
>> + static loff_t (*actor)(struct inode *, loff_t, loff_t, void *,
>> +struct iomap *);
>
> I wonder (but I'm not sure) if it would be simpler and easier to
> understand to just have to different functions for SEEK_HOLE
> vs SEEK_DATA here.

Neither variant seems obviously better to me.

Thanks,
Andreas

[Cluster-devel] [PATCH v2 44/51] gfs2: convert to bio_for_each_segment_all_sp()

[Ming Lei]

Cc: Steven Whitehouse 
Cc: Bob Peterson 
Cc: cluster-devel@redhat.com
Signed-off-by: Ming Lei 
---
 fs/gfs2/lops.c| 3 ++-
 fs/gfs2/meta_io.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index d62939f00d53..294f1926d9be 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -206,11 +206,12 @@ static void gfs2_end_log_write(struct bio *bio)
struct bio_vec *bvec;
struct page *page;
int i;
+   struct bvec_iter_all bia;
 
if (bio->bi_status)
fs_err(sdp, "Error %d writing to log\n", bio->bi_status);
 
-   bio_for_each_segment_all(bvec, bio, i) {
+   bio_for_each_segment_all_sp(bvec, bio, i, bia) {
page = bvec->bv_page;
if (page_has_buffers(page))
gfs2_end_log_write_bh(sdp, bvec, bio->bi_status);
diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index fabe1614f879..6879b0103539 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -190,8 +190,9 @@ static void gfs2_meta_read_endio(struct bio *bio)
 {
struct bio_vec *bvec;
int i;
+   struct bvec_iter_all bia;
 
-   bio_for_each_segment_all(bvec, bio, i) {
+   bio_for_each_segment_all_sp(bvec, bio, i, bia) {
struct page *page = bvec->bv_page;
struct buffer_head *bh = page_buffers(page);
unsigned int len = bvec->bv_len;
-- 
2.9.4

Re: [Cluster-devel] [PATCH v2 1/6] vfs: Add iomap_seek_hole_data helper

[Christoph Hellwig]

On Fri, Jun 23, 2017 at 03:34:39PM +0200, Andreas Gruenbacher wrote:
> Filesystems can use this for implementing lseek SEEK_HOLE / SEEK_DATA
> support via iomap.
> 
> Signed-off-by: Andreas Gruenbacher 
> ---
>  fs/iomap.c| 89 
> +++
>  include/linux/iomap.h |  3 ++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 4b10892..781f0a0 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -584,6 +584,95 @@ int iomap_fiemap(struct inode *inode, struct 
> fiemap_extent_info *fi,
>  }
>  EXPORT_SYMBOL_GPL(iomap_fiemap);
>  
> +static loff_t
> +iomap_seek_hole_actor(struct inode *inode, loff_t offset, loff_t length,
> +   void *data, struct iomap *iomap)
> +{
> + if (iomap->type == IOMAP_HOLE)
> + goto found;
> + length = iomap->offset + iomap->length - offset;

What is the problem with the passed in length value?

> + if (iomap->type != IOMAP_UNWRITTEN)
> + return length;
> + offset = page_cache_seek_hole_data(inode, offset, length, SEEK_HOLE);
> + if (offset < 0)
> + return length;
> +found:
> + *(loff_t *)data = offset;
> + return 0;

What about using a switch statement?

switch (iomap->type) {
case IOMAP_UNWRITTEN:
offset = page_cache_seek_hole_data(inode, offset, length,
SEEK_HOLE);
if (offset < 0)
return length;
/*FALLTHRU*/
case IOMAP_HOLE:
*(loff_t *)data = offset;
return 0;
default:
return length;
}

> +static loff_t
> +iomap_seek_data_actor(struct inode *inode, loff_t offset, loff_t length,
> +   void *data, struct iomap *iomap)
> +{
> + if (iomap->type != IOMAP_HOLE && iomap->type != IOMAP_UNWRITTEN)
> + goto found;
> + length = iomap->offset + iomap->length - offset;
> + if (iomap->type != IOMAP_UNWRITTEN)
> + return length;
> + offset = page_cache_seek_hole_data(inode, offset, length, SEEK_DATA);
> + if (offset < 0)
> + return length;
> +found:
> + *(loff_t *)data = offset;
> + return 0;

> +loff_t
> +iomap_seek_hole_data(struct inode *inode, loff_t offset,
> +  int whence, const struct iomap_ops *ops)
> +{
> + static loff_t (*actor)(struct inode *, loff_t, loff_t, void *,
> +struct iomap *);

I wonder (but I'm not sure) if it would be simpler and easier to
understand to just have to different functions for SEEK_HOLE
vs SEEK_DATA here.

Re: [Cluster-devel] [PATCH v2 0/6] SEEK_HOLE / SEEK_DATA via iomap

[Christoph Hellwig]

On Fri, Jun 23, 2017 at 03:34:38PM +0200, Andreas Gruenbacher wrote:
> These patches, on top of the page_cache_seek_hole_data patches posted earlier
> today, convert xfs to implement SEEK_HOLE / SEEK_DATA via iomap, and implement
> SEEK_HOLE / SEEK_DATA via iomap in gfs2.

Please send those as one series - in fact to me the previous
series doesn't even make sense - just introduce page_cache_seek_hole_data
for the iomap code only as everyone shoulkd be using that.

> ext4 doesn't implement IOMAP_REPORT, so it can't be switched to use the iomap
> based SEEK_HOLE / SEEK_DATA or fiemap code so far.

Just treat it as a no-op, it only really matters for the reflink case.