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
On Mar 14, 2008 21:02 +0900, Akira Fujita wrote:
> 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 */
> };
Would it be possible to use the FIEMAP interface to get the file mapping
data to userspace? That would avoid the need to have a second ioctl
with nearly the same data.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Hi Andreas,
>> 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 */
>> };
>>
>
> Would it be possible to use the FIEMAP interface to get the file mapping
> data to userspace? That would avoid the need to have a second ioctl
> with nearly the same data.
>
Reducing ioctl makes sense.
But EXT4_EXTENTS_INFO needs to get the file mapping data
with inode number not file name.
So we should keep ext4_extents_info structure
or put ino entry in fiemap structure.
Regards, Akira
On Mar 17, 2008 15:28 +0900, Akira Fujita wrote:
> Hi Andreas,
>>> 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 */
>>> };
>>
>> Would it be possible to use the FIEMAP interface to get the file mapping
>> data to userspace? That would avoid the need to have a second ioctl
>> with nearly the same data.
>
> Reducing ioctl makes sense.
> But EXT4_EXTENTS_INFO needs to get the file mapping data
> with inode number not file name.
> So we should keep ext4_extents_info structure
> or put ino entry in fiemap structure.
We introduced a special ioctl for this which would allow root to perform
ioctl() on any inode by inode number. We call this EXT4_IOC_WRAPPER, and
it is called on the root inode and specifies the inode number and real
ioctl number before the rest of the ioctl data.
struct ext4_ioctl_wrapper {
__u32 ioctl_cmd;
__u32 padding;
__u64 ioctl_ino;
char ioctl_data[0];
};
More patch is available, but it isn't in a self-contained patch.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
> On Mar 17, 2008 15:28 +0900, Akira Fujita wrote:
>
>> Hi Andreas,
>>
>>>> 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 */
>>>> };
>>>>
>>> Would it be possible to use the FIEMAP interface to get the file mapping
>>> data to userspace? That would avoid the need to have a second ioctl
>>> with nearly the same data.
>>>
>>
>> Reducing ioctl makes sense.
>> But EXT4_EXTENTS_INFO needs to get the file mapping data
>> with inode number not file name.
>> So we should keep ext4_extents_info structure
>> or put ino entry in fiemap structure.
>>
>
> We introduced a special ioctl for this which would allow root to perform
> ioctl() on any inode by inode number. We call this EXT4_IOC_WRAPPER, and
> it is called on the root inode and specifies the inode number and real
> ioctl number before the rest of the ioctl data.
>
> struct ext4_ioctl_wrapper {
> __u32 ioctl_cmd;
> __u32 padding;
> __u64 ioctl_ino;
> char ioctl_data[0];
> };
>
> More patch is available, but it isn't in a self-contained patch.
>
>
Is EXT4_IOC_FIEMAP going to be added to the ext4 patch queue?
I will try to use EXT4_IOC_FIEMAP instead of EXT4_IOC_EXTENTS_INFO
in ext4 online defrag when kernel supports it.
Regards, Akira
On Mon, Mar 24, 2008 at 08:00:54PM +0900, Akira Fujita wrote:
>>
>>
> Is EXT4_IOC_FIEMAP going to be added to the ext4 patch queue?
> I will try to use EXT4_IOC_FIEMAP instead of EXT4_IOC_EXTENTS_INFO
> in ext4 online defrag when kernel supports it.
Can you also look at the sparse warnings with the patches ?
-aneesh
Akira Fujita wrote:
> Is EXT4_IOC_FIEMAP going to be added to the ext4 patch queue?
> I will try to use EXT4_IOC_FIEMAP instead of EXT4_IOC_EXTENTS_INFO
> in ext4 online defrag when kernel supports it.
It's not, yet, but I could probably put it there.
It is not ready for upstream yet at all, but it does have a good amount
of functionality.
I'll try to get something submitted for the patch queue soon...
(I decided to slow down a bit on the fiemap work, and concentrate on
helping to get e2fsprogs ready with ext4 support)
-Eric
Eric Sandeen wrote:
> Akira Fujita wrote:
>
>
>> Is EXT4_IOC_FIEMAP going to be added to the ext4 patch queue?
>> I will try to use EXT4_IOC_FIEMAP instead of EXT4_IOC_EXTENTS_INFO
>> in ext4 online defrag when kernel supports it.
>>
>
> It's not, yet, but I could probably put it there.
>
>
I see.
> It is not ready for upstream yet at all, but it does have a good amount
> of functionality.
>
> I'll try to get something submitted for the patch queue soon...
>
> (I decided to slow down a bit on the fiemap work, and concentrate on
> helping to get e2fsprogs ready with ext4 support)
>
>
No problem since e2fsprogs is one of the most important things for ext4.
Regards, Akira