diff mbox

[11/34] iomap: move IOMAP_F_BOUNDARY to gfs2

Message ID 20180523144357.18985-12-hch@lst.de (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Christoph Hellwig May 23, 2018, 2:43 p.m. UTC
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>
---
 fs/gfs2/bmap.c        | 8 +++++---
 include/linux/iomap.h | 9 +++++++--
 2 files changed, 12 insertions(+), 5 deletions(-)

Comments

Darrick J. Wong May 30, 2018, 5:50 a.m. UTC | #1
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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Whitehouse May 30, 2018, 9:30 a.m. UTC | #2
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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig May 30, 2018, 9:59 a.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Whitehouse May 30, 2018, 10:02 a.m. UTC | #4
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.


--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig May 30, 2018, 10:10 a.m. UTC | #5
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Whitehouse May 30, 2018, 10:12 a.m. UTC | #6
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.


--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher May 30, 2018, 11:03 a.m. UTC | #7
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
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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:
  */