2015-04-08 17:08:57

by Eric Whitney

[permalink] [raw]
Subject: [PATCH] ext4: minor cleanup of ext4_da_reserve_space()

Remove outdated comments and dead code from ext4_da_reserve_space.
Clean up its trace point, and relocate it to make it more useful.

While we're at it, fix a nearby conditional used to determine if
we have a non-bigalloc file system. It doesn't match usage elsewhere
in the code, and misleadingly suggests that an s_cluster_ratio value
of 0 would be legal.

Signed-off-by: Eric Whitney <[email protected]>
---
fs/ext4/inode.c | 22 +++++-----------------
include/trace/events/ext4.h | 10 ++++------
2 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7eb70b7..fecc1d0 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1262,13 +1262,12 @@ static int ext4_journalled_write_end(struct file *file,
}

/*
- * Reserve a single cluster located at lblock
+ * Reserve space for a single cluster
*/
-static int ext4_da_reserve_space(struct inode *inode, ext4_lblk_t lblock)
+static int ext4_da_reserve_space(struct inode *inode)
{
struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
struct ext4_inode_info *ei = EXT4_I(inode);
- unsigned int md_needed;
int ret;

/*
@@ -1280,25 +1279,14 @@ static int ext4_da_reserve_space(struct inode *inode, ext4_lblk_t lblock)
if (ret)
return ret;

- /*
- * recalculate the amount of metadata blocks to reserve
- * in order to allocate nrblocks
- * worse case is one extent per block
- */
spin_lock(&ei->i_block_reservation_lock);
- /*
- * ext4_calc_metadata_amount() has side effects, which we have
- * to be prepared undo if we fail to claim space.
- */
- md_needed = 0;
- trace_ext4_da_reserve_space(inode, 0);
-
if (ext4_claim_free_clusters(sbi, 1, 0)) {
spin_unlock(&ei->i_block_reservation_lock);
dquot_release_reservation_block(inode, EXT4_C2B(sbi, 1));
return -ENOSPC;
}
ei->i_reserved_data_blocks++;
+ trace_ext4_da_reserve_space(inode);
spin_unlock(&ei->i_block_reservation_lock);

return 0; /* success */
@@ -1567,9 +1555,9 @@ add_delayed:
* then we don't need to reserve it again. However we still need
* to reserve metadata for every block we're going to write.
*/
- if (EXT4_SB(inode->i_sb)->s_cluster_ratio <= 1 ||
+ if (EXT4_SB(inode->i_sb)->s_cluster_ratio == 1 ||
!ext4_find_delalloc_cluster(inode, map->m_lblk)) {
- ret = ext4_da_reserve_space(inode, iblock);
+ ret = ext4_da_reserve_space(inode);
if (ret) {
/* not enough space to reserve */
retval = ret;
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 6e5abd6..6ed16df 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -1185,15 +1185,14 @@ TRACE_EVENT(ext4_da_update_reserve_space,
);

TRACE_EVENT(ext4_da_reserve_space,
- TP_PROTO(struct inode *inode, int md_needed),
+ TP_PROTO(struct inode *inode),

- TP_ARGS(inode, md_needed),
+ TP_ARGS(inode),

TP_STRUCT__entry(
__field( dev_t, dev )
__field( ino_t, ino )
__field( __u64, i_blocks )
- __field( int, md_needed )
__field( int, reserved_data_blocks )
__field( int, reserved_meta_blocks )
__field( __u16, mode )
@@ -1203,18 +1202,17 @@ TRACE_EVENT(ext4_da_reserve_space,
__entry->dev = inode->i_sb->s_dev;
__entry->ino = inode->i_ino;
__entry->i_blocks = inode->i_blocks;
- __entry->md_needed = md_needed;
__entry->reserved_data_blocks = EXT4_I(inode)->i_reserved_data_blocks;
__entry->reserved_meta_blocks = EXT4_I(inode)->i_reserved_meta_blocks;
__entry->mode = inode->i_mode;
),

- TP_printk("dev %d,%d ino %lu mode 0%o i_blocks %llu md_needed %d "
+ TP_printk("dev %d,%d ino %lu mode 0%o i_blocks %llu "
"reserved_data_blocks %d reserved_meta_blocks %d",
MAJOR(__entry->dev), MINOR(__entry->dev),
(unsigned long) __entry->ino,
__entry->mode, __entry->i_blocks,
- __entry->md_needed, __entry->reserved_data_blocks,
+ __entry->reserved_data_blocks,
__entry->reserved_meta_blocks)
);

--
2.1.0



2015-06-22 01:25:48

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] ext4: minor cleanup of ext4_da_reserve_space()

On Wed, Apr 08, 2015 at 01:08:53PM -0400, Eric Whitney wrote:
> Remove outdated comments and dead code from ext4_da_reserve_space.
> Clean up its trace point, and relocate it to make it more useful.
>
> While we're at it, fix a nearby conditional used to determine if
> we have a non-bigalloc file system. It doesn't match usage elsewhere
> in the code, and misleadingly suggests that an s_cluster_ratio value
> of 0 would be legal.
>
> Signed-off-by: Eric Whitney <[email protected]>

Applied, thanks.

- Ted