Re: [PATCH 1/5] ext4 online defrag header file changes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mingming,

> Overall I think the header changes probably should go with the patches
> that need those changes, that helps explain why we need this header
> changes and makes the patch compile itself. Right now this patch failed
> to compile alone.
I see.
I will reorganize my patches into meaningful groups.

>>  /*
>> @@ -299,6 +300,14 @@ struct ext4_new_group_data {
>>  #define EXT4_IOC_GETRSVSZ		_IOR('f', 5, long)
>>  #define EXT4_IOC_SETRSVSZ		_IOW('f', 6, long)
>>  #define EXT4_IOC_MIGRATE		_IO('f', 7)
>> +#define EXT4_IOC_FIBMAP			_IOW('f', 9, ext4_fsblk_t)
>> +#define EXT4_IOC_DEFRAG		_IOW('f', 10, struct ext4_ext_defrag_data)
>> +#define EXT4_IOC_GROUP_INFO	_IOW('f', 11, struct ext4_group_data_info)
>> +#define EXT4_IOC_FREE_BLOCKS_INFO	_IOW('f', 12, struct ext4_extents_info)
>> +#define EXT4_IOC_EXTENTS_INFO		_IOW('f', 13, struct ext4_extents_info)
>> +#define EXT4_IOC_RESERVE_BLOCK		_IOW('f', 14, struct ext4_extents_info)
>> +#define EXT4_IOC_MOVE_VICTIM		_IOW('f', 15, struct ext4_extents_info)
>> +#define EXT4_IOC_BLOCK_RELEASE		_IO('f', 8)
>
> These should go with the last patch in this series, where the ioctl
> commands get implemented
OK.

> Could you add more comments about the tunables below? What they are used
> for?
>
>> +/* Used for defrag */
>> +#define DEFRAG_MAX_ENT         32
>
> This means at most 32 free space extents per block group?
OK.  I will add more comments later.

This means the maximum count of extents(or free space extents) for exchanging 
between kernel-space and user-space at once.
For example, EXT4_IOC_EXTENTS_INFO is called multiple times(per DEFRAG_MAX_ENT)
to get its block distribution if there is a large number of
extents(or free space extents) in the target block group.

>> +#define DEFRAG_FORCE_TRY       1
>> +#define DEFRAG_FORCE_VICTIM    2
>> +#define DEFRAG_FORCE_GATHER    3
>> +
>
> And these tunables are used in the last patch in this series, so it make
> sense to move there too.
OK.

>> +struct ext4_extent_data {
>> +	ext4_lblk_t block;		/* start logical block number */
>> +	ext4_fsblk_t start;		/* start physical block number */
>> +	int len;			/* blocks count */
>> +};
>> +
>
> Not related to defrag, but I would like to consider this as in-core
> extent structure. Maybe we should use this structure in other extents.c,
> instead of sharing the same on-disk extent structure, which needs to
> worry about little endian?

I agree. 
How about renaming this structure from ext4_extent_data to
ext4_extent_info which used the extent in-core structure
and put it into ext4_fs_extents.h? 
Because in-core super_block is ext4_sb_info.

/*
 * This is the extent in-core structure.
 */
struct ext4_extent_info {
	ext4_lblk_t	ee_block;	/* first logical block extent covers */
	int		ee_len;		/* number of blocks covered by extent */
	ext4_fsblk_t	ee_start;	/* first physical block extent covers */
};      

Regards, Akira
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux