On Thu, Jun 05, 2008 at 12:28:30PM -0600, Andreas Dilger wrote:
> On Jun 05, 2008 11:37 -0400, Theodore Ts'o wrote:
> > This is better, but it still means that we are exporting a large
> > number of functions to the callers. It's not clear to me we need so
> > many different variants of ext4_new_blocks_* --- what is their
> > justification to exist?
> >
> > For example, why not just have:
> >
> > static ext4_fsblk_t ext4_new_blocks(handle_t *handle, struct inode *inode,
> > ext4_lblk_t iblock, ext4_fsblk_t goal,
> > unsigned long *count, int *errp, int meta)
> >
> > where if inode is NULL, then you're allocating a metadata block, and
> > if count is NULL, then you only want one block. Of course, this needs
> > to be carefully documented at the function.
>
> I don't necessarily agree that meta should be implied by inode != NULL.
> We do want to cluster metadata allocations for a single inode if possible,
> so keeping the inode information is useful. We may want to keep a separate
> "metadata goal block" from the "data goal block" in the inode...
>
> That said, it seems you still have a "meta" parameter here? I always hate
> having an int for a boolean, and we may as well make this a "flags" so
> that when we want to improve it later we don't need to rename it and change
> all of the "1" parameters to "EXT4_META_BLOCK". Do it right the first time.
>
how about ?
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index c6e6a6f..014e5b5 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -1928,9 +1928,11 @@ ext4_fsblk_t ext4_orlov_new_blocks(handle_t *handle, struct inode *inode,
return 0;
}
+#defin EXT4_META_BLOCK 0x1
+
static ext4_fsblk_t do_blk_alloc(handle_t *handle, struct inode *inode,
ext4_lblk_t iblock, ext4_fsblk_t goal,
- unsigned long *count, int *errp, int meta)
+ unsigned long *count, int *errp, int flags)
{
struct ext4_allocation_request ar;
ext4_fsblk_t ret;
@@ -1950,7 +1952,7 @@ static ext4_fsblk_t do_blk_alloc(handle_t *handle, struct inode *inode,
ar.goal = goal;
ar.len = *count;
ar.logical = iblock;
- if (S_ISREG(inode->i_mode) && !meta)
+ if (S_ISREG(inode->i_mode) && !(flags & EXT4_META_BLOCK))
ar.flags = EXT4_MB_HINT_DATA;
else
/* disable in-core preallocation for non-regular files */
@@ -1965,13 +1967,15 @@ ext4_fsblk_t ext4_new_meta_block(handle_t *handle, struct inode *inode,
ext4_fsblk_t goal, int *errp)
{
unsigned long count = 1;
- return do_blk_alloc(handle, inode, 0, goal, &count, errp, 1);
+ return do_blk_alloc(handle, inode, 0, goal, &count,
+ errp, EXT4_META_BLOCK);
}
ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode,
ext4_fsblk_t goal, unsigned long *count, int *errp)
{
- return do_blk_alloc(handle, inode, 0, goal, count, errp, 1);
+ return do_blk_alloc(handle, inode, 0, goal, count,
+ errp, EXT4_META_BLOCK);
}
ext4_fsblk_t ext4_new_blocks(handle_t *handle, struct inode *inode,
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
