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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <200803141202.AA00330@TNESG9526.rs.jp.nec.com>
Date:	Fri, 14 Mar 2008 21:02:18 +0900
From:	Akira Fujita <a-fujita@...jp.nec.com>
To:	cmm@...ibm.com, tytso@....edu
Cc:	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 1/5] ext4 online defrag header file changes

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-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html