[Cluster-devel] [PATCH 09/34] iomap: inline data should be an iomap type, not a flag

[Cluster-devel] [PATCH 09/34] iomap: inline data should be an iomap type, not a flag

[Darrick J. Wong]

On Wed, May 23, 2018 at 04:43:32PM +0200, Christoph Hellwig wrote:
> Inline data is fundamentally different from our normal mapped case in that
> it doesn't even have a block address.  So instead of having a flag for it
> it should be an entirely separate iomap range type.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok to me, anyone from gfs2/ext4 want to ack this?
Let's cc those lists and see what happens...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/ext4/inline.c      |  4 ++--
>  fs/gfs2/bmap.c        |  3 +--
>  fs/iomap.c            | 21 ++++++++++++---------
>  include/linux/iomap.h |  2 +-
>  4 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 70cf4c7b268a..e1f00891ef95 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -1835,8 +1835,8 @@ int ext4_inline_data_iomap(struct inode *inode, struct iomap *iomap)
>  	iomap->offset = 0;
>  	iomap->length = min_t(loff_t, ext4_get_inline_size(inode),
>  			      i_size_read(inode));
> -	iomap->type = 0;
> -	iomap->flags = IOMAP_F_DATA_INLINE;
> +	iomap->type = IOMAP_INLINE;
> +	iomap->flags = 0;
>  
>  out:
>  	up_read(&EXT4_I(inode)->xattr_sem);
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index 278ed0869c3c..cbeedd3cfb36 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -680,8 +680,7 @@ static void gfs2_stuffed_iomap(struct inode *inode, struct iomap *iomap)
>  		      sizeof(struct gfs2_dinode);
>  	iomap->offset = 0;
>  	iomap->length = i_size_read(inode);
> -	iomap->type = IOMAP_MAPPED;
> -	iomap->flags = IOMAP_F_DATA_INLINE;
> +	iomap->type = IOMAP_INLINE;
>  }
>  
>  /**
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 0900da23172c..f52209a2c270 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -503,10 +503,13 @@ static int iomap_to_fiemap(struct fiemap_extent_info *fi,
>  	case IOMAP_DELALLOC:
>  		flags |= FIEMAP_EXTENT_DELALLOC | FIEMAP_EXTENT_UNKNOWN;
>  		break;
> +	case IOMAP_MAPPED:
> +		break;
>  	case IOMAP_UNWRITTEN:
>  		flags |= FIEMAP_EXTENT_UNWRITTEN;
>  		break;
> -	case IOMAP_MAPPED:
> +	case IOMAP_INLINE:
> +		flags |= FIEMAP_EXTENT_DATA_INLINE;
>  		break;
>  	}
>  
> @@ -514,8 +517,6 @@ static int iomap_to_fiemap(struct fiemap_extent_info *fi,
>  		flags |= FIEMAP_EXTENT_MERGED;
>  	if (iomap->flags & IOMAP_F_SHARED)
>  		flags |= FIEMAP_EXTENT_SHARED;
> -	if (iomap->flags & IOMAP_F_DATA_INLINE)
> -		flags |= FIEMAP_EXTENT_DATA_INLINE;
>  
>  	return fiemap_fill_next_extent(fi, iomap->offset,
>  			iomap->addr != IOMAP_NULL_ADDR ? iomap->addr : 0,
> @@ -1318,14 +1319,16 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos,
>  	struct iomap_swapfile_info *isi = data;
>  	int error;
>  
> -	/* No inline data. */
> -	if (iomap->flags & IOMAP_F_DATA_INLINE) {
> +	switch (iomap->type) {
> +	case IOMAP_MAPPED:
> +	case IOMAP_UNWRITTEN:
> +		/* Only real or unwritten extents. */
> +		break;
> +	case IOMAP_INLINE:
> +		/* No inline data. */
>  		pr_err("swapon: file is inline\n");
>  		return -EINVAL;
> -	}
> -
> -	/* Only real or unwritten extents. */
> -	if (iomap->type != IOMAP_MAPPED && iomap->type != IOMAP_UNWRITTEN) {
> +	default:
>  		pr_err("swapon: file has unallocated extents\n");
>  		return -EINVAL;
>  	}
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 4bd87294219a..8f7095fc514e 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -18,6 +18,7 @@ struct vm_fault;
>  #define IOMAP_DELALLOC	0x02	/* delayed allocation blocks */
>  #define IOMAP_MAPPED	0x03	/* blocks allocated at @addr */
>  #define IOMAP_UNWRITTEN	0x04	/* blocks allocated at @addr in unwritten state */
> +#define IOMAP_INLINE	0x05	/* data inline in the inode */
>  
>  /*
>   * Flags for all iomap mappings:
> @@ -34,7 +35,6 @@ struct vm_fault;
>   */
>  #define IOMAP_F_MERGED		0x10	/* contains multiple blocks/extents */
>  #define IOMAP_F_SHARED		0x20	/* block shared with another file */
> -#define IOMAP_F_DATA_INLINE	0x40	/* data inline in the inode */
>  
>  /*
>   * Magic value for addr:
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Cluster-devel] [PATCH 11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

[Darrick J. Wong]

On Wed, May 23, 2018 at 04:43:34PM +0200, Christoph Hellwig wrote:
> Just define a range of fs specific flags and use that in gfs2 instead of
> exposing this internal flag flobally.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok to me, but better if the gfs2 folks [cc'd now] ack this...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/gfs2/bmap.c        | 8 +++++---
>  include/linux/iomap.h | 9 +++++++--
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index cbeedd3cfb36..8efa6297e19c 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -683,6 +683,8 @@ static void gfs2_stuffed_iomap(struct inode *inode, struct iomap *iomap)
>  	iomap->type = IOMAP_INLINE;
>  }
>  
> +#define IOMAP_F_GFS2_BOUNDARY IOMAP_F_PRIVATE
> +
>  /**
>   * gfs2_iomap_begin - Map blocks from an inode to disk blocks
>   * @inode: The inode
> @@ -774,7 +776,7 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
>  	bh = mp.mp_bh[ip->i_height - 1];
>  	len = gfs2_extent_length(bh->b_data, bh->b_size, ptr, lend - lblock, &eob);
>  	if (eob)
> -		iomap->flags |= IOMAP_F_BOUNDARY;
> +		iomap->flags |= IOMAP_F_GFS2_BOUNDARY;
>  	iomap->length = (u64)len << inode->i_blkbits;
>  
>  out_release:
> @@ -846,12 +848,12 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
>  
>  	if (iomap.length > bh_map->b_size) {
>  		iomap.length = bh_map->b_size;
> -		iomap.flags &= ~IOMAP_F_BOUNDARY;
> +		iomap.flags &= ~IOMAP_F_GFS2_BOUNDARY;
>  	}
>  	if (iomap.addr != IOMAP_NULL_ADDR)
>  		map_bh(bh_map, inode->i_sb, iomap.addr >> inode->i_blkbits);
>  	bh_map->b_size = iomap.length;
> -	if (iomap.flags & IOMAP_F_BOUNDARY)
> +	if (iomap.flags & IOMAP_F_GFS2_BOUNDARY)
>  		set_buffer_boundary(bh_map);
>  	if (iomap.flags & IOMAP_F_NEW)
>  		set_buffer_new(bh_map);
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 13d19b4c29a9..819e0cd2a950 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -27,8 +27,7 @@ struct vm_fault;
>   * written data and requires fdatasync to commit them to persistent storage.
>   */
>  #define IOMAP_F_NEW		0x01	/* blocks have been newly allocated */
> -#define IOMAP_F_BOUNDARY	0x02	/* mapping ends at metadata boundary */
> -#define IOMAP_F_DIRTY		0x04	/* uncommitted metadata */
> +#define IOMAP_F_DIRTY		0x02	/* uncommitted metadata */
>  
>  /*
>   * Flags that only need to be reported for IOMAP_REPORT requests:
> @@ -36,6 +35,12 @@ struct vm_fault;
>  #define IOMAP_F_MERGED		0x10	/* contains multiple blocks/extents */
>  #define IOMAP_F_SHARED		0x20	/* block shared with another file */
>  
> +/*
> + * Flags from 0x1000 up are for file system specific usage:
> + */
> +#define IOMAP_F_PRIVATE		0x1000
> +
> +
>  /*
>   * Magic value for addr:
>   */
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Cluster-devel] [PATCH 11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

[Steven Whitehouse]

Hi,


On 30/05/18 06:50, Darrick J. Wong wrote:
> On Wed, May 23, 2018 at 04:43:34PM +0200, Christoph Hellwig wrote:
>> Just define a range of fs specific flags and use that in gfs2 instead of
>> exposing this internal flag flobally.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Looks ok to me, but better if the gfs2 folks [cc'd now] ack this...
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> --D
I may have missed the context here, but I thought that the boundary was 
a generic thing meaning "there will have to be a metadata read before 
more blocks can be mapped" so I'm not sure why that would now be GFS2 
specific?

Steve.

>> ---
>>   fs/gfs2/bmap.c        | 8 +++++---
>>   include/linux/iomap.h | 9 +++++++--
>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
>> index cbeedd3cfb36..8efa6297e19c 100644
>> --- a/fs/gfs2/bmap.c
>> +++ b/fs/gfs2/bmap.c
>> @@ -683,6 +683,8 @@ static void gfs2_stuffed_iomap(struct inode *inode, struct iomap *iomap)
>>   	iomap->type = IOMAP_INLINE;
>>   }
>>   
>> +#define IOMAP_F_GFS2_BOUNDARY IOMAP_F_PRIVATE
>> +
>>   /**
>>    * gfs2_iomap_begin - Map blocks from an inode to disk blocks
>>    * @inode: The inode
>> @@ -774,7 +776,7 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
>>   	bh = mp.mp_bh[ip->i_height - 1];
>>   	len = gfs2_extent_length(bh->b_data, bh->b_size, ptr, lend - lblock, &eob);
>>   	if (eob)
>> -		iomap->flags |= IOMAP_F_BOUNDARY;
>> +		iomap->flags |= IOMAP_F_GFS2_BOUNDARY;
>>   	iomap->length = (u64)len << inode->i_blkbits;
>>   
>>   out_release:
>> @@ -846,12 +848,12 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
>>   
>>   	if (iomap.length > bh_map->b_size) {
>>   		iomap.length = bh_map->b_size;
>> -		iomap.flags &= ~IOMAP_F_BOUNDARY;
>> +		iomap.flags &= ~IOMAP_F_GFS2_BOUNDARY;
>>   	}
>>   	if (iomap.addr != IOMAP_NULL_ADDR)
>>   		map_bh(bh_map, inode->i_sb, iomap.addr >> inode->i_blkbits);
>>   	bh_map->b_size = iomap.length;
>> -	if (iomap.flags & IOMAP_F_BOUNDARY)
>> +	if (iomap.flags & IOMAP_F_GFS2_BOUNDARY)
>>   		set_buffer_boundary(bh_map);
>>   	if (iomap.flags & IOMAP_F_NEW)
>>   		set_buffer_new(bh_map);
>> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
>> index 13d19b4c29a9..819e0cd2a950 100644
>> --- a/include/linux/iomap.h
>> +++ b/include/linux/iomap.h
>> @@ -27,8 +27,7 @@ struct vm_fault;
>>    * written data and requires fdatasync to commit them to persistent storage.
>>    */
>>   #define IOMAP_F_NEW		0x01	/* blocks have been newly allocated */
>> -#define IOMAP_F_BOUNDARY	0x02	/* mapping ends at metadata boundary */
>> -#define IOMAP_F_DIRTY		0x04	/* uncommitted metadata */
>> +#define IOMAP_F_DIRTY		0x02	/* uncommitted metadata */
>>   
>>   /*
>>    * Flags that only need to be reported for IOMAP_REPORT requests:
>> @@ -36,6 +35,12 @@ struct vm_fault;
>>   #define IOMAP_F_MERGED		0x10	/* contains multiple blocks/extents */
>>   #define IOMAP_F_SHARED		0x20	/* block shared with another file */
>>   
>> +/*
>> + * Flags from 0x1000 up are for file system specific usage:
>> + */
>> +#define IOMAP_F_PRIVATE		0x1000
>> +
>> +
>>   /*
>>    * Magic value for addr:
>>    */
>> -- 
>> 2.17.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Cluster-devel] [PATCH 11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

[Christoph Hellwig]

On Wed, May 30, 2018 at 10:30:32AM +0100, Steven Whitehouse wrote:
> I may have missed the context here, but I thought that the boundary was a 
> generic thing meaning "there will have to be a metadata read before more 
> blocks can be mapped" so I'm not sure why that would now be GFS2 specific?

It was always a hack.  But with iomap it doesn't make any sensee to start
with, all metadata I/O happens in iomap_begin, so there is no point in
marking an iomap with flags like this for the actual iomap interface.

[Cluster-devel] [PATCH 11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

[Steven Whitehouse]

Hi,


On 30/05/18 10:59, Christoph Hellwig wrote:
> On Wed, May 30, 2018 at 10:30:32AM +0100, Steven Whitehouse wrote:
>> I may have missed the context here, but I thought that the boundary was a
>> generic thing meaning "there will have to be a metadata read before more
>> blocks can be mapped" so I'm not sure why that would now be GFS2 specific?
> It was always a hack.  But with iomap it doesn't make any sensee to start
> with, all metadata I/O happens in iomap_begin, so there is no point in
> marking an iomap with flags like this for the actual iomap interface.

In that case,? maybe it would be simpler to drop it for GFS2. Unless we 
are getting a lot of benefit from it, then we should probably just 
follow the generic pattern here. Eventually we'll move everything to 
iomap, so that the bh mapping interface will be gone. That implies that 
we might be able to drop it now, to avoid this complication during the 
conversion.

Andreas, do you see any issues with that?

Steve.

[Cluster-devel] [PATCH 11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

[Christoph Hellwig]

On Wed, May 30, 2018 at 11:02:08AM +0100, Steven Whitehouse wrote:
> In that case,? maybe it would be simpler to drop it for GFS2. Unless we 
> are getting a lot of benefit from it, then we should probably just follow 
> the generic pattern here. Eventually we'll move everything to iomap, so 
> that the bh mapping interface will be gone. That implies that we might be 
> able to drop it now, to avoid this complication during the conversion.
>
> Andreas, do you see any issues with that?

I suspect it actually is doing the wrong thing today.  It certainly
does for SSDs, and it probably doesn't do a useful thing for modern
disks with intelligent caches either.

[Cluster-devel] [PATCH 11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

[Steven Whitehouse]

Hi,


On 30/05/18 11:10, Christoph Hellwig wrote:
> On Wed, May 30, 2018 at 11:02:08AM +0100, Steven Whitehouse wrote:
>> In that case,? maybe it would be simpler to drop it for GFS2. Unless we
>> are getting a lot of benefit from it, then we should probably just follow
>> the generic pattern here. Eventually we'll move everything to iomap, so
>> that the bh mapping interface will be gone. That implies that we might be
>> able to drop it now, to avoid this complication during the conversion.
>>
>> Andreas, do you see any issues with that?
> I suspect it actually is doing the wrong thing today.  It certainly
> does for SSDs, and it probably doesn't do a useful thing for modern
> disks with intelligent caches either.

Yes, agreed that it makes no sense for SSDs,

Steve.

[Cluster-devel] [PATCH 11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

[Andreas Gruenbacher]

On 30 May 2018 at 12:12, Steven Whitehouse <swhiteho@redhat.com> wrote:
> Hi,
>
> On 30/05/18 11:10, Christoph Hellwig wrote:
>>
>> On Wed, May 30, 2018 at 11:02:08AM +0100, Steven Whitehouse wrote:
>>>
>>> In that case,  maybe it would be simpler to drop it for GFS2. Unless we
>>> are getting a lot of benefit from it, then we should probably just follow
>>> the generic pattern here. Eventually we'll move everything to iomap, so
>>> that the bh mapping interface will be gone. That implies that we might be
>>> able to drop it now, to avoid this complication during the conversion.
>>>
>>> Andreas, do you see any issues with that?

We're not handling reads through iomap yet, so I'd be happier with
keeping that flag in one form or the other until we get there. This
will go away eventually anyway.

>> I suspect it actually is doing the wrong thing today.  It certainly
>> does for SSDs, and it probably doesn't do a useful thing for modern
>> disks with intelligent caches either.
>
>
> Yes, agreed that it makes no sense for SSDs,

Thanks,
Andreas