On Thu, Mar 31, 2011 at 11:59:38AM -0300, Davidlohr Bueso wrote:
> #define INODE_SIZE (sizeof(struct minix_inode))
>
> #define INODE_SIZE2 (sizeof(struct minix2_inode))
> -#define INODE_BLOCKS UPPER(INODES, (version2 ? MINIX2_INODES_PER_BLOCK \
> - : MINIX_INODES_PER_BLOCK))
> -#define INODE_BUFFER_SIZE (INODE_BLOCKS * BLOCK_SIZE)
> +#define INODE_BLOCKS UPPER(INODES, inodes_per_block)
>
> -#define BITS_PER_BLOCK (BLOCK_SIZE<<3)
> +#define INODE_BUFFER_SIZE (INODE_BLOCKS * BLOCK_SIZE)
>
> -#define MAX_INODES 65535
> +#define BITS_PER_BLOCK (blksz<<3)
[...]
> #define Super (*(struct minix_super_block *)super_block_buffer)
> -#define INODES ((unsigned long)Super.s_ninodes)
> -#define ZONES ((unsigned long)(version2 ? Super.s_zones : Super.s_nzones))
> -#define IMAPS ((unsigned long)Super.s_imap_blocks)
> -#define ZMAPS ((unsigned long)Super.s_zmap_blocks)
> -#define FIRSTZONE ((unsigned long)Super.s_firstdatazone)
> -#define ZONESIZE ((unsigned long)Super.s_log_zone_size)
> -#define MAXSIZE ((unsigned long)Super.s_max_size)
> +#define Super3 (*(struct minix3_super_block *)super_block_buffer)
> +#define INODES ((unsigned long)(fs_version == 3 ? Super3.s_ninodes : Super.s_ninodes))
> +#define ZONES ((unsigned long)(fs_version == 3 ? Super3.s_zones : (fs_version == 2 ? Super.s_zones : Super.s_nzones)))
> +#define IMAPS ((unsigned long)(fs_version == 3 ? Super3.s_imap_blocks : Super.s_imap_blocks))
> +#define ZMAPS ((unsigned long)(fs_version == 3 ? Super3.s_zmap_blocks : Super.s_zmap_blocks))
> +#define FIRSTZONE ((unsigned long) (fs_version == 3 ? Super3.s_firstdatazone : Super.s_firstdatazone))
> +#define ZONESIZE ((unsigned long)(fs_version == 3 ? Super3.s_log_zone_size : Super.s_log_zone_size))
> +#define MAXSIZE ((unsigned long)(fs_version == 3 ? Super3.s_max_size : Super.s_max_size))
> #define NORM_FIRSTZONE (2+IMAPS+ZMAPS+INODE_BLOCKS)
It would be better to rewrite all these crazy macros to small inline
functions and share this code between minix mkfs and fsck code.
> static char *inode_map;
> @@ -141,6 +157,8 @@ static unsigned long req_nr_inodes = 0;
> #define mark_zone(x) (setbit(zone_map,(x)-FIRSTZONE+1))
> #define unmark_zone(x) (clrbit(zone_map,(x)-FIRSTZONE+1))
>
> +#define INODE_MAX 65535
> +
> static void
> die(char *str) {
> fprintf(stderr, "%s: ", program_name);
> @@ -149,10 +167,13 @@ die(char *str) {
> exit(8);
> }
>
> -static void __attribute__((__noreturn__))
> +static void
> usage(void) {
> - errx(16, _("Usage: %s [-c | -l filename] [-nXX] [-iXX] /dev/name [blocks]"),
> - program_name);
> + fprintf(stderr, _("%s (%s)\n"), program_name, PACKAGE_STRING);
Why? I think that our normal usage() functions are without PACKAGE_STRING.
> + fprintf(stderr,
> + _("Usage: %s [-c | -l filename] [-nXX] [-iXX] /dev/name [blocks]\n"),
> + program_name);
> + exit(16);
> }
>
> /*
> @@ -177,23 +198,28 @@ check_mount(void) {
> die(_("%s is mounted; will not make a filesystem here!"));
> }
>
> -static void
> +static void
> write_tables(void) {
> /* Mark the super block valid. */
> - Super.s_state |= MINIX_VALID_FS;
> - Super.s_state &= ~MINIX_ERROR_FS;
> -
> + if (fs_version == 3) {
> + Super3.s_state |= MINIX_VALID_FS;
> + Super3.s_state &= ~MINIX_ERROR_FS;
> + } else {
> + Super.s_state |= MINIX_VALID_FS;
> + Super.s_state &= ~MINIX_ERROR_FS;
> + }
int *state = super_get_state_ptr()
*state |= MINIX_VALID_FS;
*state &= ~MINIX_ERROR_FS
> if (lseek(DEV, 0, SEEK_SET))
> die(_("seek to boot block failed in write_tables"));
> if (512 != write(DEV, boot_block_buffer, 512))
> die(_("unable to clear boot sector"));
> - if (BLOCK_SIZE != lseek(DEV, BLOCK_SIZE, SEEK_SET))
> + if (blksz != lseek(DEV, blksz, SEEK_SET))
> die(_("seek failed in write_tables"));
> - if (BLOCK_SIZE != write(DEV, super_block_buffer, BLOCK_SIZE))
> + if (blksz != write(DEV, super_block_buffer, blksz))
> die(_("unable to write super-block"));
> - if (IMAPS*BLOCK_SIZE != write(DEV,inode_map,IMAPS*BLOCK_SIZE))
> + if (IMAPS*blksz != write(DEV,inode_map,IMAPS*blksz))
> die(_("unable to write inode map"));
> - if (ZMAPS*BLOCK_SIZE != write(DEV,zone_map,ZMAPS*BLOCK_SIZE))
> + if (ZMAPS*blksz != write(DEV,zone_map,ZMAPS*blksz))
> die(_("unable to write zone map"));
> if (INODE_BUFFER_SIZE != write(DEV,inode_buffer,INODE_BUFFER_SIZE))
> die(_("unable to write inodes"));
> @@ -202,9 +228,9 @@ write_tables(void) {
>
> static void
> write_block(int blk, char * buffer) {
> - if (blk*BLOCK_SIZE != lseek(DEV, blk*BLOCK_SIZE, SEEK_SET))
> + if (blk*blksz != lseek(DEV, blk*blksz, SEEK_SET))
> die(_("seek failed in write_block"));
> - if (BLOCK_SIZE != write(DEV, buffer, BLOCK_SIZE))
> + if (blksz != write(DEV, buffer, blksz))
> die(_("write failed in write_block"));
> }
>
> @@ -250,8 +276,8 @@ make_bad_inode(void) {
> struct minix_inode * inode = &Inode[MINIX_BAD_INO];
> int i,j,zone;
> int ind=0,dind=0;
> - unsigned short ind_block[BLOCK_SIZE>>1];
> - unsigned short dind_block[BLOCK_SIZE>>1];
> + unsigned short ind_block[blksz>>1];
> + unsigned short dind_block[blksz>>1];
I think that we need another patch that will clean up the coding
style -- for example space around operators ( block[blksz >> 1] ).
> #define NEXT_BAD (zone = next(zone))
>
> @@ -261,7 +287,7 @@ make_bad_inode(void) {
> inode->i_nlinks = 1;
> inode->i_time = time(NULL);
> inode->i_mode = S_IFREG + 0000;
> - inode->i_size = badblocks*BLOCK_SIZE;
> + inode->i_size = badblocks*blksz;
> zone = next(0);
> for (i=0 ; i<7 ; i++) {
> inode->i_zone[i] = zone;
> @@ -269,18 +295,18 @@ make_bad_inode(void) {
> goto end_bad;
> }
> inode->i_zone[7] = ind = get_free_block();
> - memset(ind_block,0,BLOCK_SIZE);
> + memset(ind_block,0,blksz);
> for (i=0 ; i<512 ; i++) {
> ind_block[i] = zone;
> if (!NEXT_BAD)
> goto end_bad;
> }
> inode->i_zone[8] = dind = get_free_block();
> - memset(dind_block,0,BLOCK_SIZE);
> + memset(dind_block,0,blksz);
> for (i=0 ; i<512 ; i++) {
> write_block(ind,(char *) ind_block);
> dind_block[i] = ind = get_free_block();
> - memset(ind_block,0,BLOCK_SIZE);
> + memset(ind_block,0,blksz);
> for (j=0 ; j<512 ; j++) {
> ind_block[j] = zone;
> if (!NEXT_BAD)
> @@ -296,12 +322,13 @@ end_bad:
> }
>
> static void
> -make_bad_inode2 (void) {
> +make_bad_inode2 (void)
> +{
> struct minix2_inode *inode = &Inode2[MINIX_BAD_INO];
> int i, j, zone;
> int ind = 0, dind = 0;
> - unsigned long ind_block[BLOCK_SIZE >> 2];
> - unsigned long dind_block[BLOCK_SIZE >> 2];
> + unsigned long ind_block[blksz >> 2];
> + unsigned long dind_block[blksz >> 2];
>
> if (!badblocks)
> return;
> @@ -309,7 +336,7 @@ make_bad_inode2 (void) {
> inode->i_nlinks = 1;
> inode->i_atime = inode->i_mtime = inode->i_ctime = time (NULL);
> inode->i_mode = S_IFREG + 0000;
> - inode->i_size = badblocks * BLOCK_SIZE;
> + inode->i_size = badblocks * blksz;
> zone = next (0);
> for (i = 0; i < 7; i++) {
> inode->i_zone[i] = zone;
> @@ -317,18 +344,18 @@ make_bad_inode2 (void) {
> goto end_bad;
> }
> inode->i_zone[7] = ind = get_free_block ();
> - memset (ind_block, 0, BLOCK_SIZE);
> + memset (ind_block, 0, blksz);
> for (i = 0; i < 256; i++) {
> ind_block[i] = zone;
> if (!NEXT_BAD)
> goto end_bad;
> }
> inode->i_zone[8] = dind = get_free_block ();
> - memset (dind_block, 0, BLOCK_SIZE);
> + memset (dind_block, 0, blksz);
> for (i = 0; i < 256; i++) {
> write_block (ind, (char *) ind_block);
> dind_block[i] = ind = get_free_block ();
> - memset (ind_block, 0, BLOCK_SIZE);
> + memset (ind_block, 0, blksz);
> for (j = 0; j < 256; j++) {
> ind_block[j] = zone;
> if (!NEXT_BAD)
> @@ -337,7 +364,7 @@ make_bad_inode2 (void) {
> }
> /* Could make triple indirect block here */
> die (_("too many bad blocks"));
> - end_bad:
> +end_bad:
> if (ind)
> write_block (ind, (char *) ind_block);
> if (dind)
> @@ -345,9 +372,13 @@ make_bad_inode2 (void) {
> }
>
> static void
> -make_root_inode(void) {
> +make_root_inode(void)
> +{
> struct minix_inode * inode = &Inode[MINIX_ROOT_INO];
>
> +
> +
> +
> mark_inode(MINIX_ROOT_INO);
> inode->i_zone[0] = get_free_block();
> inode->i_nlinks = 2;
> @@ -363,126 +394,154 @@ make_root_inode(void) {
> inode->i_uid = getuid();
> if (inode->i_uid)
> inode->i_gid = getgid();
> - write_block(inode->i_zone[0],root_block);
> + write_block(inode->i_zone[0], root_block);
> }
>
> -static void
> -make_root_inode2 (void) {
> +static void
> +make_root_inode2 (void)
This is for minix 2 and 3, what about to rename it (and also
make_bad_inode())?
make_root_inode_v1();
make_root_inode_v2_v3();
make_root_inode()
{
if (fs_version < 2)
return make_root_inode_v1()
return make_root_inode_v2_v3();
}
> +{
> struct minix2_inode *inode = &Inode2[MINIX_ROOT_INO];
>
> mark_inode (MINIX_ROOT_INO);
> inode->i_zone[0] = get_free_block ();
> inode->i_nlinks = 2;
> inode->i_atime = inode->i_mtime = inode->i_ctime = time (NULL);
> +
> if (badblocks)
> inode->i_size = 3 * dirsize;
> else {
> root_block[2 * dirsize] = '\0';
> - root_block[2 * dirsize + 1] = '\0';
^^^^
> - inode->i_size = 2 * dirsize;
> + root_block[2 * dirsize] = '\0';
Why '+1' is unnecessary here?
> + if (fs_version == 2)
> + inode->i_size = 2 * dirsize;
> }
> inode->i_mode = S_IFDIR + 0755;
> inode->i_uid = getuid();
> if (inode->i_uid)
> inode->i_gid = getgid();
> +
> write_block (inode->i_zone[0], root_block);
> }
>
> -static void
> -setup_tables(void) {
> +/* sets up the boot sector, superblock and inode tables */
> +static void
> +setup_tables(void)
> +{
> int i;
> unsigned long inodes;
>
> - super_block_buffer = calloc(1, BLOCK_SIZE);
> - if (!super_block_buffer)
> - die(_("unable to alloc buffer for superblock"));
> -
> + super_block_buffer = xcalloc(1, blksz);
> memset(boot_block_buffer,0,512);
> - Super.s_magic = magic;
> - Super.s_log_zone_size = 0;
> - Super.s_max_size = version2 ? 0x7fffffff : (7+512+512*512)*1024;
> - if (version2)
> +
> + if (fs_version == 3) {
> + Super3.s_magic = magic;
> + Super3.s_log_zone_size = 0;
> + Super3.s_blocksize = blksz;
> + }
> +
> + else {
> + Super.s_magic = magic;
> + Super.s_log_zone_size = 0;
> + }
> +
> + if (fs_version == 3)
> + Super3.s_max_size = 2147483647L;
> + else
> + Super.s_max_size = fs_version == 2 ? 0x7fffffff : (7+512+512*512)*1024;
Your coding style is inconsistent :-)
if (fs_version == 3)
....
else if (fs_version == 2)
....
else
....
or super_init_maxsize()
> + if (fs_version == 3)
> + Super3.s_zones = BLOCKS;
> + else if (fs_version == 2)
> Super.s_zones = BLOCKS;
> else
> Super.s_nzones = BLOCKS;
super_set_nzones(BLOCKS);
>
> -/* some magic nrs: 1 inode / 3 blocks */
> + /* some magic nrs: 1 inode / 3 blocks */
> if ( req_nr_inodes == 0 )
> inodes = BLOCKS/3;
> else
> inodes = req_nr_inodes;
> /* Round up inode count to fill block size */
> - if (version2)
> + if (fs_version == 2 || fs_version == 3)
> inodes = ((inodes + MINIX2_INODES_PER_BLOCK - 1) &
> ~(MINIX2_INODES_PER_BLOCK - 1));
> else
> inodes = ((inodes + MINIX_INODES_PER_BLOCK - 1) &
> ~(MINIX_INODES_PER_BLOCK - 1));
> - if (inodes > 65535)
> - inodes = 65535;
> - Super.s_ninodes = inodes;
> -
> - /* The old code here
> - * ZMAPS = 0;
> - * while (ZMAPS != UPPER(BLOCKS - NORM_FIRSTZONE + 1,BITS_PER_BLOCK))
> - * ZMAPS = UPPER(BLOCKS - NORM_FIRSTZONE + 1,BITS_PER_BLOCK);
> - * was no good, since it may loop. - aeb
> - */
> - Super.s_imap_blocks = UPPER(INODES + 1, BITS_PER_BLOCK);
> - Super.s_zmap_blocks = UPPER(BLOCKS - (1+IMAPS+INODE_BLOCKS),
> - BITS_PER_BLOCK+1);
> - Super.s_firstdatazone = NORM_FIRSTZONE;
> -
> - inode_map = malloc(IMAPS * BLOCK_SIZE);
> - zone_map = malloc(ZMAPS * BLOCK_SIZE);
> - if (!inode_map || !zone_map)
> - die(_("unable to allocate buffers for maps"));
> - memset(inode_map,0xff,IMAPS * BLOCK_SIZE);
> - memset(zone_map,0xff,ZMAPS * BLOCK_SIZE);
> +
> + if (fs_version == 3)
> + Super3.s_ninodes = inodes;
> + else {
> + if (inodes > INODE_MAX)
> + inodes = INODE_MAX;
> + Super.s_ninodes = inodes;
> + }
> +
> + if (fs_version == 3)
> + Super3.s_imap_blocks = UPPER(INODES + 1, BITS_PER_BLOCK);
> + else
> + Super.s_imap_blocks = UPPER(INODES + 1, BITS_PER_BLOCK);
> +
> + if (fs_version == 3)
> + Super3.s_zmap_blocks = UPPER(BLOCKS - (1+IMAPS+INODE_BLOCKS),
> + BITS_PER_BLOCK+1);
> + else
> + Super.s_zmap_blocks = UPPER(BLOCKS - (1+IMAPS+INODE_BLOCKS),
> + BITS_PER_BLOCK+1);
> +
> + if (fs_version == 3)
> + Super3.s_firstdatazone = NORM_FIRSTZONE;
> + else
> + Super.s_firstdatazone = NORM_FIRSTZONE;
super_set_imap_blocks(...);
super_set_zmap_blocks(...);
super_set_firstdatazone(...);
> + inode_map = xmalloc(IMAPS * blksz);
> + zone_map = xmalloc(ZMAPS * blksz);
> + memset(inode_map,0xff,IMAPS * blksz);
> + memset(zone_map,0xff,ZMAPS * blksz);
> for (i = FIRSTZONE ; i<ZONES ; i++)
> unmark_zone(i);
> for (i = MINIX_ROOT_INO ; i<=INODES ; i++)
> unmark_inode(i);
> - inode_buffer = malloc(INODE_BUFFER_SIZE);
> - if (!inode_buffer)
> - die(_("unable to allocate buffer for inodes"));
> +
> + inode_buffer = xmalloc(INODE_BUFFER_SIZE);
> memset(inode_buffer,0,INODE_BUFFER_SIZE);
Use xcalloc(), remove memset().
> - printf(_("%ld inodes\n"),INODES);
> - printf(_("%ld blocks\n"),ZONES);
> - printf(_("Firstdatazone=%ld (%ld)\n"),FIRSTZONE,NORM_FIRSTZONE);
> - printf(_("Zonesize=%d\n"),BLOCK_SIZE<<ZONESIZE);
> - printf(_("Maxsize=%ld\n\n"),MAXSIZE);
> +
> + printf(_("%lu inodes\n"), INODES);
> + printf(_("%ld blocks\n"), ZONES);
> + printf(_("Firstdatazone=%ld (%ld)\n"), FIRSTZONE, NORM_FIRSTZONE);
> + printf(_("Zonesize=%llu\n"), blksz<<ZONESIZE);
> + printf(_("Maxsize=%ld\n\n"), MAXSIZE);
> }
>
> /*
> * Perform a test of a block; return the number of
> * blocks readable/writeable.
> */
> -static long
> -do_check(char * buffer, int try, unsigned int current_block) {
> +static long do_check(char * buffer, int try, unsigned int current_block)
> +{
> long got;
>
> /* Seek to the correct loc. */
> - if (lseek(DEV, current_block * BLOCK_SIZE, SEEK_SET) !=
> - current_block * BLOCK_SIZE ) {
> - die(_("seek failed during testing of blocks"));
> + if (lseek(DEV, current_block * blksz, SEEK_SET) !=
> + current_block * blksz ) {
> + die(_("seek failed during testing of blocks"));
> }
I think we need another patch with stuff from err.h to replace
die().
> /* Try the read */
> - got = read(DEV, buffer, try * BLOCK_SIZE);
> + got = read(DEV, buffer, try * blksz);
> if (got < 0) got = 0;
> - if (got & (BLOCK_SIZE - 1 )) {
> + if (got & (blksz - 1 )) {
> printf(_("Weird values in do_check: probably bugs\n"));
> }
> - got /= BLOCK_SIZE;
> + got /= blksz;
> return got;
> }
>
> static unsigned int currently_testing = 0;
>
> -static void
> -alarm_intr(int alnum) {
> +static void alarm_intr(int alnum)
> +{
> if (currently_testing >= ZONES)
> return;
> signal(SIGALRM,alarm_intr);
> @@ -493,8 +552,8 @@ alarm_intr(int alnum) {
> fflush(stdout);
> }
>
> -static void
> -check_blocks(void) {
> +static void check_blocks(void)
> +{
> int try,got;
> static char buffer[BLOCK_SIZE * TEST_BUFFER_BLOCKS];
>
> @@ -502,8 +561,8 @@ check_blocks(void) {
> signal(SIGALRM,alarm_intr);
> alarm(5);
> while (currently_testing < ZONES) {
> - if (lseek(DEV,currently_testing*BLOCK_SIZE,SEEK_SET) !=
> - currently_testing*BLOCK_SIZE)
> + if (lseek(DEV,currently_testing*blksz,SEEK_SET) !=
> + currently_testing*blksz)
> die(_("seek failed in check_blocks"));
> try = TEST_BUFFER_BLOCKS;
> if (currently_testing + try > ZONES)
> @@ -549,13 +608,10 @@ get_list_blocks(char *filename) {
> printf(_("one bad block\n"));
> }
>
> -int
> -main(int argc, char ** argv) {
> - int i;
> - char * tmp;
> +int main(int argc, char ** argv) {
> struct stat statbuf;
> - char * listfile = NULL;
> - char * p;
> + char *tmp, *p, *listfile = NULL;
> + int i, sectorsize; /* physical sector size in bytes */
No, blkdev_get_sector_size() returns _logical_ sector size (usually
512).
...
> if (S_ISBLK(statbuf.st_mode)) {
> - int sectorsize;
> -
> if (blkdev_get_sector_size(DEV, §orsize) == -1)
> die(_("cannot determine sector size for %s"));
> if (BLOCK_SIZE < sectorsize)
> @@ -660,29 +720,62 @@ main(int argc, char ** argv) {
> check=0;
> } else if (statbuf.st_rdev == 0x0300 || statbuf.st_rdev == 0x0340)
> die(_("will not try to make filesystem on '%s'"));
> +
> + /* determine what block size to use, based on fs version and user input */
> + if (blksz && fs_version == 3) {
> + if (blksz%sectorsize || blksz < BLOCK_SIZE) {
if (blksz % sectorsize || blksz < BLOCK_SIZE) {
> + fprintf(stderr, "block size must be multiple of sector (%d) "
> + "and at least %d bytes\n", sectorsize, BLOCK_SIZE);
> + die(_("specified block size illegal"));
> + }
> + if(blksz%INODE_SIZE2) {
again, use space around operators
> + fprintf(stderr, "block size must be a multiple of inode size (%lu bytes)\n",
> + INODE_SIZE2);
> + die(_("specified block size illegal"));
> + }
> + }
> + else /* use the default block size */
> + blksz = BLOCK_SIZE;
It means that for 4K disk you have to manually specify the size by
"-B 4096", right?
This is unnecessary, because kernel (since 2.6.32) provides all information
about block device I/O limits.
https://ata.wiki.kernel.org/index.php/ATA_4_KiB_sector_issues
http://people.redhat.com/msnitzer/docs/io-limits.txt
The default for v3 should be based on BLKPBSZGET ioctl (probably add
blkdev_get_physector_size() to lib/blkdev.c), if this is impossible
then fall back to BLOCK_SIZE.
I think that well designed mkfs program should be also check if the
device is not misaligned -- the alignment offset (BLKALIGNOFF) has to
be zero. For misaligned devices we need to print a warning (hmm..
this feature is missing in mkswap(8) too) -- maybe we need a new
lib/blkdev.c function blkdev_is_aligned().
You can try scsi_debug kernel module to create 4K disks:
# Create "desktop-class" 4K drive
# (logical_block_size=512, physical_block_size=4096, alignment_offset=0):
modprobe scsi_debug dev_size_mb=$DEV_SIZE sector_size=512 physblk_exp=3
# Create "desktop-class" 4K drive w/ 63-sector DOS partition compensation
# (logical_block_size=512, physical_block_size=4096, alignment_offset=3584):
modprobe scsi_debug dev_size_mb=$DEV_SIZE sector_size=512 physblk_exp=3 lowest_aligned=7
# Create "enterprise-class" 4K drive
# (logical_block_size=4096, physical_block_size=4096, alignment_offset=0):
modprobe scsi_debug dev_size_mb=$DEV_SIZE sector_size=4096
Oh.. Davidlohr, you forgot regression test ;-) What about to add
mkfs.minix -{1,2,3}
to tests/ts/minix/mkfs{1,2,3} ?
It would be also nice to add MINIX3_SUPER_MAGIC to fsck.minix and
print a warning that v3 is unsupported (yet?).
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
