Hi Ted, Thank you for comments. Theodore Tso wrote: > On Thu, Nov 06, 2008 at 10:19:50AM +0900, Akira Fujita wrote: >> Thank you for fixing, but there is a problem. >> ac_excepted_group has not only excepted block group number >> but also -1 (it means any block groups are accepted). >> So I think it is necessary for defrag to keep it as "long long", >> because if maximum number of the ext4_group_t is set excepted_group, >> defrag can't handle block group number correctly. > > It's a really bad idea from a portability perspective to use either > long, unsigned long, or long long directly. On some architecture, who > knows what size long long might be; it might be a 128 bit integer on > some future system. The better way to do this is to allocate a > EXT4_MB_HINT_ flag which indicates whether ac_excepted_group is valid, > and then let ac_excepted_group have the correct type. I see. I will make ac_excepted_group have only block group number. And create "#define EXT4_MB_ANY_BG 2048" flag which means any block group are accepted (equivalent to former ac_excepted_group = -1) instead. > Looking at defrag-08-add-ioc-move-victim-ioctl, I'm still concerned > that we have far too much policy in the kernel-side code. The fact > that there is "phases" for the ioctl seems very wrong. You don't > normally find that in normal system calls, since it implies the kernel > is dictating how various system calls will be used, and in what order. Do you mean that it is not good EXT4_IOC_DEFRAG ioctl's behavior is changed by "phases"? If so, is it OK to create new ioctls equivalent to "phases" (EXT4_IOC_DEFRAG_FORCE_XXX), for example? > I'll note that there isn't that much difference between defragging an > inode where you don't constrain where the blocks go, and defragging an > inode where you want the blocks to go in a specific range of blocks > (which I wouldn't necessarily constrain to a single block group; a > range of blocks would be more general), and defragging an inode where > you specify the range where you *don't* want the blocks to go, is all > the same thing, except for the type of constraint you place on the new > blocks during the inode block migration operation. > > So when I approach this from a kernel system call API design > perspective, I start thinking about a data structure where the user > program can specify some kind of constraint (or possibly multiple > constraints, although that adds more complexities) and attach it to a > file descriptor (perhaps via fcntl), and then *all* allocations, > regardless of whether it is defrag, or block allocations, would be > affected by the constraint. > > Do you see what I mean? The kernel should provide general purpose > primitive building blocks, which can be used in multiple ways by > different userspace applications. So by factoring out what needs to > be done in each of the phases, it's possible to create a relatively > small/simple system call and/or ioctl extension that modifies or > extends the existing functions without encoding application specific > detail into the kernel. > > - Ted Regards, Akira Fujita -- 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
