RE: [PATCH 02/16 v2] f2fs: add on-disk layout

[Jaegeuk Kim]

[snip]
> > +#define F2FS_SUPER_MAGIC	0xF2F52010
> > +#define F2FS_SUPER_OFFSET	0		/* start sector # for sb */
> 
> Does f2fs superblock really haven't any offset from the volume begin?

The reason that I changed this from 1 to 0 is due to the failure during android
recovery. I don't know why the recovery is failed when the offset is 1, but it
works fine after the offset is changed to 0.
I suspect that mount procedure inspects the 0'th offset to figure out what file
system is installed by checking some kind of magic numbers.
Sometimes, we've seen that the mount program tries to load previously installed
file system even though mkfs.f2fs was conducted.
Would you recommend something?

> 
> > +#define F2FS_BLKSIZE		4096
> > +#define F2FS_MAX_EXTENSION	64
> > +
> > +#define NULL_ADDR		0x0U
> > +#define NEW_ADDR		-1U
> 
> Does NULL_ADDR and NEW_ADDR declarations really need? Does kernel
> haven't any analogous?

These are used for F2FS-specific block allocation, so for readability,
I don't want to change this.

> 
> > +
> > +#define F2FS_ROOT_INO(sbi)	(sbi->root_ino_num)
> > +#define F2FS_NODE_INO(sbi)	(sbi->node_ino_num)
> > +#define F2FS_META_INO(sbi)	(sbi->meta_ino_num)
> > +
> > +#define GFP_F2FS_MOVABLE	(__GFP_WAIT | __GFP_IO | __GFP_ZERO)
> > +
> > +#define MAX_ACTIVE_LOGS	16
> > +#define MAX_ACTIVE_NODE_LOGS	8
> > +#define MAX_ACTIVE_DATA_LOGS	8
> 
> I think that it makes sense to comment the reasons of such limitations
> in MAX_ACTIVE_LOGS, MAX_ACTIVE_NODE_LOGS, MAX_ACTIVE_DATA_LOGS.

The maximum number of logs is suggested by arnd before.
As I understood, why he suggested such a quite large number is for further
optimization of multiple logs without any on-disk layout changes.
And, I think it is quite enough.

> 
> > +
> > +/*
> > + * For superblock
> > + */
> > +struct f2fs_super_block {
> > +	__le32 magic;		/* Magic Number */
> > +	__le16 major_ver;	/* Major Version */
> > +	__le16 minor_ver;	/* Minor Version */
> > +	__le32 log_sectorsize;	/* log2 (Sector size in bytes) */
> > +	__le32 log_sectors_per_block;	/* log2 (Number of sectors per block */
> > +	__le32 log_blocksize;	/* log2 (Block size in bytes) */
> > +	__le32 log_blocks_per_seg; /* log2 (Number of blocks per segment) */
> 
> From my point of view, __le32 is big data type for log2 (<value>). What
> do you think?
> 

Right, but it is superblock. Should we have to consider space overhead?

> > +	__le32 segs_per_sec;	/* Number of segments per section */
> > +	__le32 secs_per_zone;	/* Number of sections per zone */
> > +	__le32 checksum_offset;	/* Checksum position in this super block */
> > +	__le64 block_count;	/* Total number of blocks */
> > +	__le32 section_count;	/* Total number of sections */
> > +	__le32 segment_count;	/* Total number of segments */
> > +	__le32 segment_count_ckpt; /* Total number of segments
> > +				      in Checkpoint area */
> > +	__le32 segment_count_sit; /* Total number of segments
> > +				     in Segment information table */
> > +	__le32 segment_count_nat; /* Total number of segments
> > +				     in Node address table */
> > +	/*Total number of segments in Segment summary area */
> > +	__le32 segment_count_ssa;
> > +	/* Total number of segments in Main area */
> > +	__le32 segment_count_main;
> > +	__le32 failure_safe_block_distance;
> > +	__le32 segment0_blkaddr;	/* Start block address of Segment 0 */
> > +	__le32 start_segment_checkpoint; /* Start block address of ckpt */
> > +	__le32 sit_blkaddr;	/* Start block address of SIT */
> > +	__le32 nat_blkaddr;	/* Start block address of NAT */
> > +	__le32 ssa_blkaddr;     /* Start block address of SSA */
> > +	__le32 main_blkaddr;	/* Start block address of Main area */
> > +	__le32 root_ino;	/* Root directory inode number */
> > +	__le32 node_ino;	/* node inode number */
> > +	__le32 meta_ino;	/* meta inode number */
> > +	__le32 volume_serial_number;	/* VSN is optional field */
> 
> Usually, it is used 128-bits UUID for serial number. Why do you use
> __le32 as volume_serial_number?

Ok, I'll change.

[snip]
> > +/*
> > + * For directory operations
> > + */
> > +#define F2FS_DOT_HASH		0
> > +#define F2FS_DDOT_HASH		F2FS_DOT_HASH
> > +#define F2FS_MAX_HASH		(~((0x3ULL) << 62))
> > +#define F2FS_HASH_COL_BIT	((0x1ULL) << 63)
> > +
> > +typedef __le32	f2fs_hash_t;
> > +
> > +#define F2FS_NAME_LEN		8
> 
> It exists F2FS_MAX_NAME_LEN. I think that it makes sense to comment here
> purpose of F2FS_NAME_LEN declaration.

Ok, thanks.

---
Jaegeuk Kim
Samsung


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/