| Message ID | 8b4499e47bea3841194850e1b3eeb924d87e69a5.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:33:09, Matthew Bobrowski wrote: > Separate the iomap field population chunk into a separate simple > helper routine. This helps reducing the overall clutter within the > ext4_iomap_begin() callback, especially as we move across more code to > make use of iomap infrastructure. > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> > --- > fs/ext4/inode.c | 65 ++++++++++++++++++++++++++++--------------------- > 1 file changed, 37 insertions(+), 28 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 516faa280ced..1ccdc14c4d69 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3406,10 +3406,43 @@ static bool ext4_inode_datasync_dirty(struct inode *inode) > return inode->i_state & I_DIRTY_DATASYNC; > } > > +static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > + unsigned long first_block, struct ext4_map_blocks *map) > +{ > + u8 blkbits = inode->i_blkbits; > + > + iomap->flags = 0; > + if (ext4_inode_datasync_dirty(inode)) > + iomap->flags |= IOMAP_F_DIRTY; > + iomap->bdev = inode->i_sb->s_bdev; > + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; > + iomap->offset = (u64) first_block << blkbits; > + iomap->length = (u64) map->m_len << blkbits; > + > + if (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) { > + iomap->type = IOMAP_UNWRITTEN; > + } else { > + WARN_ON_ONCE(1); > + return -EIO; > + } > + iomap->addr = (u64) map->m_pblk << blkbits; > + } Looking at this function now, the 'type' argument looks a bit weird. Can we perhaps just remove the 'type' argument and change the above to: 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; } And then in ext4_iomap_begin() we overwrite the type to: if (delalloc && iomap->type == IOMAP_HOLE) iomap->type = IOMAP_DELALLOC; That would IMO make ext4_set_iomap() arguments harder to get wrong. Honza > + > + if (map->m_flags & EXT4_MAP_NEW) > + iomap->flags |= IOMAP_F_NEW; > + return 0; > +} > + > 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); > + u16 type = 0; > unsigned int blkbits = inode->i_blkbits; > unsigned long first_block, last_block; > struct ext4_map_blocks map; > @@ -3523,33 +3556,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; > - > - return 0; > + if (!ret) > + type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE; > + return ext4_set_iomap(inode, iomap, type, first_block, &map); > } > > static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, > -- > 2.20.1 >
Some minor comments apart from Jan comments. On 10/3/19 5:03 PM, Matthew Bobrowski wrote: > Separate the iomap field population chunk into a separate simple > helper routine. This helps reducing the overall clutter within the > ext4_iomap_begin() callback, especially as we move across more code to > make use of iomap infrastructure. > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> > --- > fs/ext4/inode.c | 65 ++++++++++++++++++++++++++++--------------------- > 1 file changed, 37 insertions(+), 28 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 516faa280ced..1ccdc14c4d69 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3406,10 +3406,43 @@ static bool ext4_inode_datasync_dirty(struct inode *inode) > return inode->i_state & I_DIRTY_DATASYNC; > } > > +static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > + unsigned long first_block, struct ext4_map_blocks *map) Line beyond 80 chars. Please check with checkpatch once. We can also get rid of "first_block" argument here. Also the "type" argument also looks confusing. Please see comment on patch 3. > +{ > + u8 blkbits = inode->i_blkbits; > + > + iomap->flags = 0; > + if (ext4_inode_datasync_dirty(inode)) > + iomap->flags |= IOMAP_F_DIRTY; > + iomap->bdev = inode->i_sb->s_bdev; > + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; > + iomap->offset = (u64) first_block << blkbits; > + iomap->length = (u64) map->m_len << blkbits; > + > + if (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) { > + 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; > + return 0; > +} > + > 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); > + u16 type = 0; > unsigned int blkbits = inode->i_blkbits; > unsigned long first_block, last_block; > struct ext4_map_blocks map; > @@ -3523,33 +3556,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; > - > - return 0; > + if (!ret) > + type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE; > + return ext4_set_iomap(inode, iomap, type, first_block, &map); > } > > static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, >
On Wed, Oct 09, 2019 at 11:32:54AM +0530, Ritesh Harjani wrote:
> We can also get rid of "first_block" argument here.
That would just duplicate filling it out in all callers, so why?
On 10/9/19 12:37 PM, Christoph Hellwig wrote: > On Wed, Oct 09, 2019 at 11:32:54AM +0530, Ritesh Harjani wrote: >> We can also get rid of "first_block" argument here. > > That would just duplicate filling it out in all callers, so why? > What I meant is "first_block" is same as map->m_lblk. (unless ext4_map_blocks can change map->m_lblk, which AFAICT, it should not). So why pass an extra argument when we are already passing 'map' structure. So we can fill iomap->offset using map->m_lblk in ext4_set_iomap() function. -ritesh
On Tue, Oct 08, 2019 at 12:27:09PM +0200, Jan Kara wrote: > On Thu 03-10-19 21:33:09, Matthew Bobrowski wrote: > > +static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > > + unsigned long first_block, struct ext4_map_blocks *map) > > +{ > > + u8 blkbits = inode->i_blkbits; > > + > > + iomap->flags = 0; > > + if (ext4_inode_datasync_dirty(inode)) > > + iomap->flags |= IOMAP_F_DIRTY; > > + iomap->bdev = inode->i_sb->s_bdev; > > + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; > > + iomap->offset = (u64) first_block << blkbits; > > + iomap->length = (u64) map->m_len << blkbits; > > + > > + if (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) { > > + iomap->type = IOMAP_UNWRITTEN; > > + } else { > > + WARN_ON_ONCE(1); > > + return -EIO; > > + } > > + iomap->addr = (u64) map->m_pblk << blkbits; > > + } > > Looking at this function now, the 'type' argument looks a bit weird. Can we > perhaps just remove the 'type' argument and change the above to: We can, but refer to the point below. > 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; > } > > And then in ext4_iomap_begin() we overwrite the type to: > > if (delalloc && iomap->type == IOMAP_HOLE) > iomap->type = IOMAP_DELALLOC; > > That would IMO make ext4_set_iomap() arguments harder to get wrong. I was thinking about this while doing a bunch of other things at work today. I'm kind of aligned with what Christoph mentioned around possibly duplicating some of the post 'iomap->type' setting from both current and any future ext4_set_iomap() callers. In addition to this, my thought was that if we're populating the iomap structure with values respectively, then it would make most sense to encapsulate those routines, if possible, within the ext4_set_iomap() as that's the sole purpose of the function. However, no real strong objections for dropping 'type', but I just wanted to share my thoughts. Also, yes, we probably can drop 'first_block' from the list of arguments here as we can derive that from 'map' and set 'iomap->type' accordingly... --<M>--
On Wed, Oct 09, 2019 at 01:20:17PM +0530, Ritesh Harjani wrote: > On 10/9/19 12:37 PM, Christoph Hellwig wrote: > > On Wed, Oct 09, 2019 at 11:32:54AM +0530, Ritesh Harjani wrote: > > > We can also get rid of "first_block" argument here. > > > > That would just duplicate filling it out in all callers, so why? > > > > What I meant is "first_block" is same as map->m_lblk. > (unless ext4_map_blocks can change map->m_lblk, which AFAICT, it should > not). > So why pass an extra argument when we are already passing 'map' > structure. > So we can fill iomap->offset using map->m_lblk in ext4_set_iomap() > function. What you're saying makes sense Ritesh and I will update it as such. I believe what Christoph was actually referring to was what I explained to Jan to the email that I just sent out. This was around the possible code duplication and having some iomap value setting related logic outside ext4_set_iomap(), when in fact there's no real reason why it can't be inside. --<M>--
On Wed 09-10-19 19:57:23, Matthew Bobrowski wrote: > On Tue, Oct 08, 2019 at 12:27:09PM +0200, Jan Kara wrote: > > On Thu 03-10-19 21:33:09, Matthew Bobrowski wrote: > > > +static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > > > + unsigned long first_block, struct ext4_map_blocks *map) > > > +{ > > > + u8 blkbits = inode->i_blkbits; > > > + > > > + iomap->flags = 0; > > > + if (ext4_inode_datasync_dirty(inode)) > > > + iomap->flags |= IOMAP_F_DIRTY; > > > + iomap->bdev = inode->i_sb->s_bdev; > > > + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; > > > + iomap->offset = (u64) first_block << blkbits; > > > + iomap->length = (u64) map->m_len << blkbits; > > > + > > > + if (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) { > > > + iomap->type = IOMAP_UNWRITTEN; > > > + } else { > > > + WARN_ON_ONCE(1); > > > + return -EIO; > > > + } > > > + iomap->addr = (u64) map->m_pblk << blkbits; > > > + } > > > > Looking at this function now, the 'type' argument looks a bit weird. Can we > > perhaps just remove the 'type' argument and change the above to: > > We can, but refer to the point below. > > > 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; > > } > > > > And then in ext4_iomap_begin() we overwrite the type to: > > > > if (delalloc && iomap->type == IOMAP_HOLE) > > iomap->type = IOMAP_DELALLOC; > > > > That would IMO make ext4_set_iomap() arguments harder to get wrong. > > I was thinking about this while doing a bunch of other things at work > today. I'm kind of aligned with what Christoph mentioned around > possibly duplicating some of the post 'iomap->type' setting from both > current and any future ext4_set_iomap() callers. In addition to this, > my thought was that if we're populating the iomap structure with > values respectively, then it would make most sense to encapsulate > those routines, if possible, within the ext4_set_iomap() as that's the > sole purpose of the function. Well, what I dislike about 'type' argument is the inconsistency in it's handling. It is useful only for HOLE/DELALLOC, anything else will just give you invalid iomap and you have to be careful to pass 0 in that case. I understand the concern about possible duplication but since only IOMAP_REPORT cares about IOMAP_DELALLOC, I'm not much concerned about it. But another sensible API would be to optionally pass 'struct extent_status *es' argument to ext4_set_iomap() and if this argument is non-NULL, ext4_set_iomap() will handle the intersection of ext4_map_blocks and extent_status and deduce appropriate resulting type from that. Honza
On Wed, Oct 09, 2019 at 03:06:09PM +0200, Jan Kara wrote: > On Wed 09-10-19 19:57:23, Matthew Bobrowski wrote: > > On Tue, Oct 08, 2019 at 12:27:09PM +0200, Jan Kara wrote: > > > On Thu 03-10-19 21:33:09, Matthew Bobrowski wrote: > > > > +static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > > > > + unsigned long first_block, struct ext4_map_blocks *map) > > > > +{ > > > > + u8 blkbits = inode->i_blkbits; > > > > + > > > > + iomap->flags = 0; > > > > + if (ext4_inode_datasync_dirty(inode)) > > > > + iomap->flags |= IOMAP_F_DIRTY; > > > > + iomap->bdev = inode->i_sb->s_bdev; > > > > + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; > > > > + iomap->offset = (u64) first_block << blkbits; > > > > + iomap->length = (u64) map->m_len << blkbits; > > > > + > > > > + if (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) { > > > > + iomap->type = IOMAP_UNWRITTEN; > > > > + } else { > > > > + WARN_ON_ONCE(1); > > > > + return -EIO; > > > > + } > > > > + iomap->addr = (u64) map->m_pblk << blkbits; > > > > + } > > > > > > Looking at this function now, the 'type' argument looks a bit weird. Can we > > > perhaps just remove the 'type' argument and change the above to: > > > > We can, but refer to the point below. > > > > > 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; > > > } > > > > > > And then in ext4_iomap_begin() we overwrite the type to: > > > > > > if (delalloc && iomap->type == IOMAP_HOLE) > > > iomap->type = IOMAP_DELALLOC; > > > > > > That would IMO make ext4_set_iomap() arguments harder to get wrong. > > > > I was thinking about this while doing a bunch of other things at work > > today. I'm kind of aligned with what Christoph mentioned around > > possibly duplicating some of the post 'iomap->type' setting from both > > current and any future ext4_set_iomap() callers. In addition to this, > > my thought was that if we're populating the iomap structure with > > values respectively, then it would make most sense to encapsulate > > those routines, if possible, within the ext4_set_iomap() as that's the > > sole purpose of the function. > > Well, what I dislike about 'type' argument is the inconsistency in it's > handling. It is useful only for HOLE/DELALLOC, anything else will just give > you invalid iomap and you have to be careful to pass 0 in that case. > > I understand the concern about possible duplication but since only > IOMAP_REPORT cares about IOMAP_DELALLOC, I'm not much concerned about it. > But another sensible API would be to optionally pass 'struct extent_status > *es' argument to ext4_set_iomap() and if this argument is non-NULL, > ext4_set_iomap() will handle the intersection of ext4_map_blocks and > extent_status and deduce appropriate resulting type from that. I see and thank you for sharing your view point. Let's go with what you originally proposed, which is dropping the 'type' argument and then handling IOMAP_DELALLOC within ext4_set_iomap(). No real hard objections. :) --<M>--
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 516faa280ced..1ccdc14c4d69 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3406,10 +3406,43 @@ static bool ext4_inode_datasync_dirty(struct inode *inode) return inode->i_state & I_DIRTY_DATASYNC; } +static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, + unsigned long first_block, struct ext4_map_blocks *map) +{ + u8 blkbits = inode->i_blkbits; + + iomap->flags = 0; + if (ext4_inode_datasync_dirty(inode)) + iomap->flags |= IOMAP_F_DIRTY; + iomap->bdev = inode->i_sb->s_bdev; + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; + iomap->offset = (u64) first_block << blkbits; + iomap->length = (u64) map->m_len << blkbits; + + if (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) { + 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; + return 0; +} + 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); + u16 type = 0; unsigned int blkbits = inode->i_blkbits; unsigned long first_block, last_block; struct ext4_map_blocks map; @@ -3523,33 +3556,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; - - return 0; + if (!ret) + type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE; + return ext4_set_iomap(inode, iomap, type, first_block, &map); } static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
Separate the iomap field population chunk into a separate simple helper routine. This helps reducing the overall clutter within the ext4_iomap_begin() callback, especially as we move across more code to make use of iomap infrastructure. Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> --- fs/ext4/inode.c | 65 ++++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 28 deletions(-)