Message ID | 3551610e53aa1984210a4de04ad6e1a89f5bf0a3.1570100361.git.mbobrowski@mbobrowski.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ext4: port direct I/O to iomap infrastructure | expand |
On Thu 03-10-19 21:34:51, Matthew Bobrowski wrote: > For iomap direct IO write code path changes, we need to accommodate > for the case where the block mapping flags passed to ext4_map_blocks() > will result in m_flags having both EXT4_MAP_MAPPED and > EXT4_MAP_UNWRITTEN bits set. In order for the allocated unwritten > extents to be converted properly in the end_io handler, iomap->type > must be set to IOMAP_UNWRITTEN, so we need to reshuffle the > conditional statement in order to achieve this. > > This change is a no-op for DAX code path as the block mapping flag > passed to ext4_map_blocks() when IS_DAX(inode) never results in > EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN being set at once. > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> The patch looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Just those 72 columns limited comment lines... ;) Honza > --- > fs/ext4/inode.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index e133dda55063..63ad23ae05b8 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3420,10 +3420,20 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > iomap->type = type; > iomap->addr = IOMAP_NULL_ADDR; > } else { > - if (map->m_flags & EXT4_MAP_MAPPED) { > - iomap->type = IOMAP_MAPPED; > - } else if (map->m_flags & EXT4_MAP_UNWRITTEN) { > + /* > + * Flags passed to ext4_map_blocks() for direct I/O > + * writes can result in map->m_flags having both > + * EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In > + * order for allocated extents to be converted to > + * written extents in the ->end_io handler correctly, > + * we need to ensure that the iomap->type is set > + * approprately. Thus, we need to check whether > + * EXT4_MAP_UNWRITTEN is set first. > + */ > + if (map->m_flags & EXT4_MAP_UNWRITTEN) { > iomap->type = IOMAP_UNWRITTEN; > + } else if (map->m_flags & EXT4_MAP_MAPPED) { > + iomap->type = IOMAP_MAPPED; > } else { > WARN_ON_ONCE(1); > return -EIO; > -- > 2.20.1 >
On 10/3/19 5:04 PM, Matthew Bobrowski wrote: > For iomap direct IO write code path changes, we need to accommodate > for the case where the block mapping flags passed to ext4_map_blocks() > will result in m_flags having both EXT4_MAP_MAPPED and > EXT4_MAP_UNWRITTEN bits set. In order for the allocated unwritten > extents to be converted properly in the end_io handler, iomap->type > must be set to IOMAP_UNWRITTEN, so we need to reshuffle the > conditional statement in order to achieve this. > > This change is a no-op for DAX code path as the block mapping flag > passed to ext4_map_blocks() when IS_DAX(inode) never results in > EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN being set at once. > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> You may be changing the function parameters & name here, (in ext4_set_iomap) But functionality wise the patch looks good to me. You may add: Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com> > --- > fs/ext4/inode.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index e133dda55063..63ad23ae05b8 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3420,10 +3420,20 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > iomap->type = type; > iomap->addr = IOMAP_NULL_ADDR; > } else { > - if (map->m_flags & EXT4_MAP_MAPPED) { > - iomap->type = IOMAP_MAPPED; > - } else if (map->m_flags & EXT4_MAP_UNWRITTEN) { > + /* > + * Flags passed to ext4_map_blocks() for direct I/O > + * writes can result in map->m_flags having both > + * EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In > + * order for allocated extents to be converted to > + * written extents in the ->end_io handler correctly, > + * we need to ensure that the iomap->type is set > + * approprately. Thus, we need to check whether > + * EXT4_MAP_UNWRITTEN is set first. > + */ > + if (map->m_flags & EXT4_MAP_UNWRITTEN) { > iomap->type = IOMAP_UNWRITTEN; > + } else if (map->m_flags & EXT4_MAP_MAPPED) { > + iomap->type = IOMAP_MAPPED; > } else { > WARN_ON_ONCE(1); > return -EIO; >
On Tue, Oct 08, 2019 at 01:30:12PM +0200, Jan Kara wrote: > The patch looks good to me. You can add: > > Reviewed-by: Jan Kara <jack@suse.cz> Thanks Jan! > Just those 72 columns limited comment lines... ;) *nod* - will be fixed! --<M>--
On Wed, Oct 09, 2019 at 12:05:47PM +0530, Ritesh Harjani wrote: > You may be changing the function parameters & name here, > (in ext4_set_iomap) Hm, maybe. :P It all depends on the outcome of what we discuss in 1/8. > But functionality wise the patch looks good to me. > > You may add: > Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com> Great, thanks Ritesh! --<M>--
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index e133dda55063..63ad23ae05b8 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3420,10 +3420,20 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, iomap->type = type; iomap->addr = IOMAP_NULL_ADDR; } else { - if (map->m_flags & EXT4_MAP_MAPPED) { - iomap->type = IOMAP_MAPPED; - } else if (map->m_flags & EXT4_MAP_UNWRITTEN) { + /* + * Flags passed to ext4_map_blocks() for direct I/O + * writes can result in map->m_flags having both + * EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In + * order for allocated extents to be converted to + * written extents in the ->end_io handler correctly, + * we need to ensure that the iomap->type is set + * approprately. Thus, we need to check whether + * EXT4_MAP_UNWRITTEN is set first. + */ + if (map->m_flags & EXT4_MAP_UNWRITTEN) { iomap->type = IOMAP_UNWRITTEN; + } else if (map->m_flags & EXT4_MAP_MAPPED) { + iomap->type = IOMAP_MAPPED; } else { WARN_ON_ONCE(1); return -EIO;
For iomap direct IO write code path changes, we need to accommodate for the case where the block mapping flags passed to ext4_map_blocks() will result in m_flags having both EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In order for the allocated unwritten extents to be converted properly in the end_io handler, iomap->type must be set to IOMAP_UNWRITTEN, so we need to reshuffle the conditional statement in order to achieve this. This change is a no-op for DAX code path as the block mapping flag passed to ext4_map_blocks() when IS_DAX(inode) never results in EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN being set at once. Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> --- fs/ext4/inode.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)