| Message ID | 20180523144357.18985-12-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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.
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.
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.
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
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: */
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(-)