Message ID | 7dd1a1a895fd7e55c659b10bba16976faab4cd85.1571647178.git.mbobrowski@mbobrowski.org |
---|---|
State | Superseded |
Headers | show |
Series | ext4: port direct I/O to iomap infrastructure | expand |
On Mon 21-10-19 20:17:31, Matthew Bobrowski wrote: > Separate the iomap field population chunk of code that is currently > within ext4_iomap_begin() into a new helper called > ext4_set_iomap(). The intent of this function is self explanatory, > however the rationale behind doing so is to also reduce the overall > clutter that we currently have within the ext4_iomap_begin() callback. > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> > --- > fs/ext4/inode.c | 59 +++++++++++++++++++++++++++---------------------- > 1 file changed, 33 insertions(+), 26 deletions(-) The patch looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza
On 10/21/19 2:47 PM, Matthew Bobrowski wrote: > Separate the iomap field population chunk of code that is currently > within ext4_iomap_begin() into a new helper called > ext4_set_iomap(). The intent of this function is self explanatory, > however the rationale behind doing so is to also reduce the overall > clutter that we currently have within the ext4_iomap_begin() callback. > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> Could you please re-arrange patch sequence in this fashion. 1. Patch-11 (re-ordering of unwritten flags) 2. Patch-8 (trylock in IOCB_NOWAIT cases) 3. Patch-2 (should explain offset & len in this patch) 4. Patch-1 (this patch). This is so that some of these are anyway fixes or refactoring which can be picked up easily, either for backporting or sometimes this helps in getting some of the patches in, if the patch series gets bigger. Also others (like me) can also pick some of these changes then to meet their dependency. :) This patch looks good to me. You may add: Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com> > --- > fs/ext4/inode.c | 59 +++++++++++++++++++++++++++---------------------- > 1 file changed, 33 insertions(+), 26 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 516faa280ced..158eea9a1944 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3406,10 +3406,39 @@ static bool ext4_inode_datasync_dirty(struct inode *inode) > return inode->i_state & I_DIRTY_DATASYNC; > } > > +static void ext4_set_iomap(struct inode *inode, struct iomap *iomap, > + struct ext4_map_blocks *map, loff_t offset, > + loff_t length) > +{ > + u8 blkbits = inode->i_blkbits; > + > + iomap->flags = 0; > + if (ext4_inode_datasync_dirty(inode)) > + iomap->flags |= IOMAP_F_DIRTY; > + > + if (map->m_flags & EXT4_MAP_NEW) > + iomap->flags |= IOMAP_F_NEW; > + > + iomap->bdev = inode->i_sb->s_bdev; > + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; > + iomap->offset = (u64) map->m_lblk << blkbits; > + iomap->length = (u64) map->m_len << blkbits; > + > + if (map->m_flags & (EXT4_MAP_MAPPED | EXT4_MAP_UNWRITTEN)) { > + if (map->m_flags & EXT4_MAP_MAPPED) > + iomap->type = IOMAP_MAPPED; > + else if (map->m_flags & EXT4_MAP_UNWRITTEN) > + iomap->type = IOMAP_UNWRITTEN; > + iomap->addr = (u64) map->m_pblk << blkbits; > + } else { > + iomap->type = IOMAP_HOLE; > + iomap->addr = IOMAP_NULL_ADDR; > + } > +} > + > static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > unsigned flags, struct iomap *iomap) > { > - struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); > unsigned int blkbits = inode->i_blkbits; > unsigned long first_block, last_block; > struct ext4_map_blocks map; > @@ -3523,31 +3552,9 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > return ret; > } > > - iomap->flags = 0; > - if (ext4_inode_datasync_dirty(inode)) > - iomap->flags |= IOMAP_F_DIRTY; > - iomap->bdev = inode->i_sb->s_bdev; > - iomap->dax_dev = sbi->s_daxdev; > - iomap->offset = (u64)first_block << blkbits; > - iomap->length = (u64)map.m_len << blkbits; > - > - if (ret == 0) { > - iomap->type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE; > - 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) { > - iomap->type = IOMAP_UNWRITTEN; > - } else { > - WARN_ON_ONCE(1); > - return -EIO; > - } > - iomap->addr = (u64)map.m_pblk << blkbits; > - } > - > - if (map.m_flags & EXT4_MAP_NEW) > - iomap->flags |= IOMAP_F_NEW; > + ext4_set_iomap(inode, iomap, &map, offset, length); > + if (delalloc && iomap->type == IOMAP_HOLE) > + iomap->type = IOMAP_DELALLOC; > > return 0; > } >
On Wed, Oct 23, 2019 at 12:01:51PM +0530, Ritesh Harjani wrote: > > > On 10/21/19 2:47 PM, Matthew Bobrowski wrote: > > Separate the iomap field population chunk of code that is currently > > within ext4_iomap_begin() into a new helper called > > ext4_set_iomap(). The intent of this function is self explanatory, > > however the rationale behind doing so is to also reduce the overall > > clutter that we currently have within the ext4_iomap_begin() callback. > > > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> > > Could you please re-arrange patch sequence in this fashion. > > 1. Patch-11 (re-ordering of unwritten flags) > 2. Patch-8 (trylock in IOCB_NOWAIT cases) > 3. Patch-2 (should explain offset & len in this patch) > 4. Patch-1 (this patch). No objections to this. Just needing to do a little shuffle here and there. > This is so that some of these are anyway fixes or refactoring > which can be picked up easily, either for backporting or > sometimes this helps in getting some of the patches in, if the patch > series gets bigger. > Also others (like me) can also pick some of these changes then to meet > their dependency. :) Sure, thanks for educating me and making me aware of this. > This patch looks good to me. You may add: > > Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com> Thanks Ritesh! --<M>--
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 516faa280ced..158eea9a1944 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3406,10 +3406,39 @@ static bool ext4_inode_datasync_dirty(struct inode *inode) return inode->i_state & I_DIRTY_DATASYNC; } +static void ext4_set_iomap(struct inode *inode, struct iomap *iomap, + struct ext4_map_blocks *map, loff_t offset, + loff_t length) +{ + u8 blkbits = inode->i_blkbits; + + iomap->flags = 0; + if (ext4_inode_datasync_dirty(inode)) + iomap->flags |= IOMAP_F_DIRTY; + + if (map->m_flags & EXT4_MAP_NEW) + iomap->flags |= IOMAP_F_NEW; + + iomap->bdev = inode->i_sb->s_bdev; + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; + iomap->offset = (u64) map->m_lblk << blkbits; + iomap->length = (u64) map->m_len << blkbits; + + if (map->m_flags & (EXT4_MAP_MAPPED | EXT4_MAP_UNWRITTEN)) { + if (map->m_flags & EXT4_MAP_MAPPED) + iomap->type = IOMAP_MAPPED; + else if (map->m_flags & EXT4_MAP_UNWRITTEN) + iomap->type = IOMAP_UNWRITTEN; + iomap->addr = (u64) map->m_pblk << blkbits; + } else { + iomap->type = IOMAP_HOLE; + iomap->addr = IOMAP_NULL_ADDR; + } +} + static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, unsigned flags, struct iomap *iomap) { - struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); unsigned int blkbits = inode->i_blkbits; unsigned long first_block, last_block; struct ext4_map_blocks map; @@ -3523,31 +3552,9 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, return ret; } - iomap->flags = 0; - if (ext4_inode_datasync_dirty(inode)) - iomap->flags |= IOMAP_F_DIRTY; - iomap->bdev = inode->i_sb->s_bdev; - iomap->dax_dev = sbi->s_daxdev; - iomap->offset = (u64)first_block << blkbits; - iomap->length = (u64)map.m_len << blkbits; - - if (ret == 0) { - iomap->type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE; - 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) { - iomap->type = IOMAP_UNWRITTEN; - } else { - WARN_ON_ONCE(1); - return -EIO; - } - iomap->addr = (u64)map.m_pblk << blkbits; - } - - if (map.m_flags & EXT4_MAP_NEW) - iomap->flags |= IOMAP_F_NEW; + ext4_set_iomap(inode, iomap, &map, offset, length); + if (delalloc && iomap->type == IOMAP_HOLE) + iomap->type = IOMAP_DELALLOC; return 0; }
Separate the iomap field population chunk of code that is currently within ext4_iomap_begin() into a new helper called ext4_set_iomap(). The intent of this function is self explanatory, however the rationale behind doing so is to also reduce the overall clutter that we currently have within the ext4_iomap_begin() callback. Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> --- fs/ext4/inode.c | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 26 deletions(-)