[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] F2FS support
|
From: |
Daniel Kiper |
|
Subject: |
Re: [PATCH] F2FS support |
|
Date: |
Wed, 28 Mar 2018 14:04:52 +0200 |
|
User-agent: |
Mutt/1.3.28i |
On Thu, Mar 22, 2018 at 04:47:47PM +0000, Pete Batard wrote:
> From 40030514e682191281e8ddba8d1e0835e6b685dc Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <address@hidden>
> Date: Thu, 4 May 2017 19:12:00 +0100
> Subject: [PATCH] F2FS support
>
> "F2FS (Flash-Friendly File System) is flash-friendly file system which was
> merged
> into Linux kernel v3.8 in 2013.
>
> The motive for F2FS was to build a file system that from the start, takes into
> account the characteristics of NAND flash memory-based storage devices (such
> as
> solid-state disks, eMMC, and SD cards).
>
> F2FS was designed on a basis of a log-structured file system approach, which
> remedies some known issues of the older log structured file systems, such as
> the snowball effect of wandering trees and high cleaning overhead. In
> addition,
> since a NAND-based storage device shows different characteristics according to
> its internal geometry or flash memory management scheme (such as the Flash
> Translation Layer or FTL), it supports various parameters not only for
> configuring on-disk layout, but also for selecting allocation and cleaning
> algorithm.", quote by https://en.wikipedia.org/wiki/F2FS.
>
> The source codes for F2FS are available from:
>
> http://git.kernel.org/cgit/linux/kernel/git/jaegeuk/f2fs.git
> http://git.kernel.org/cgit/linux/kernel/git/jaegeuk/f2fs-tools.git
>
> Update:
> - This patch has been integrated in OpenMandriva Lx 3.
> https://www.openmandriva.org/
>
> Acked-by: Andrei Borzenkov <address@hidden>
Please drop this Acked-by. I will ask you to do some changes, mostly
nitpicks, and this means that it is no longer valid.
> Signed-off-by: Jaegeuk Kim <address@hidden>
Lack of your SOB.
[...]
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 65b4bbe..5afdc5a 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -360,7 +360,8 @@ blocklist notation. The currently supported filesystem
> types are @dfn{Amiga
> Fast FileSystem (AFFS)}, @dfn{AtheOS fs}, @dfn{BeFS},
> @dfn{BtrFS} (including raid0, raid1, raid10, gzip and lzo),
> @dfn{cpio} (little- and big-endian bin, odc and newc variants),
> address@hidden ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32}, @dfn{exFAT},
> @dfn{HFS},
> address@hidden ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32}, @dfn{exFAT},
> address@hidden, @dfn{HFS},
> @dfn{HFS+}, @dfn{ISO9660} (including Joliet, Rock-ridge and multi-chunk
> files),
I would like to see this in one line:
@dfn{exFAT}, @dfn{f2fs}, @dfn{HFS}, @dfn{HFS+},
Hmmm... s/f2fs/F2FS/?
> @dfn{JFS}, @dfn{Minix fs} (versions 1, 2 and 3), @dfn{nilfs2},
> @dfn{NTFS} (including compression), @dfn{ReiserFS}, @dfn{ROMFS},
> @@ -5375,7 +5376,7 @@ NTFS, JFS, UDF, HFS+, exFAT, long filenames in FAT,
> Joliet part of
> ISO9660 are treated as UTF-16 as per specification. AFS and BFS are read
> as UTF-8, again according to specification. BtrFS, cpio, tar, squash4, minix,
> minix2, minix3, ROMFS, ReiserFS, XFS, ext2, ext3, ext4, FAT (short names),
> -RockRidge part of ISO9660, nilfs2, UFS1, UFS2 and ZFS are assumed
> +f2fs, RockRidge part of ISO9660, nilfs2, UFS1, UFS2 and ZFS are assumed
s/f2fs/F2FS/?
> to be UTF-8. This might be false on systems configured with legacy charset
> but as long as the charset used is superset of ASCII you should be able to
> access ASCII-named files. And it's recommended to configure your system to
> use
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 2c1d62c..fc4767f 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1315,6 +1315,11 @@ module = {
> };
>
> module = {
> + name = f2fs;
> + common = fs/f2fs.c;
> +};
> +
> +module = {
> name = fshelp;
> common = fs/fshelp.c;
> };
> diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
> new file mode 100644
> index 0000000..7fb256f
> --- /dev/null
> +++ b/grub-core/fs/f2fs.c
> @@ -0,0 +1,1289 @@
> +/*
> + * f2fs.c - Flash-Friendly File System
> + *
> + * Written by Jaegeuk Kim <address@hidden>
> + *
> + * Copyright (C) 2015 Free Software Foundation, Inc.
> + *
> + * GRUB is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * GRUB is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> + */
Lack of empty line.
> +#include <grub/err.h>
> +#include <grub/file.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +#include <grub/disk.h>
> +#include <grub/dl.h>
> +#include <grub/types.h>
> +#include <grub/charset.h>
> +#include <grub/fshelp.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +/* F2FS Magic Number */
> +#define F2FS_SUPER_MAGIC 0xF2F52010
> +#define CHECKSUM_OFFSET 4092 /* must be aligned 4
> bytes */
> +#define U32_CHECKSUM_OFFSET (CHECKSUM_OFFSET >> 2)
> +
> +/* byte-size offset */
> +#define F2FS_SUPER_OFFSET ((grub_disk_addr_t)1024)
> +#define F2FS_SUPER_OFFSET0 (F2FS_SUPER_OFFSET >> GRUB_DISK_SECTOR_BITS)
> +#define F2FS_SUPER_OFFSET1 ((F2FS_SUPER_OFFSET + F2FS_BLKSIZE) >> \
Redundant space before "\".
> + GRUB_DISK_SECTOR_BITS)
> +
> +/* 9 bits for 512 bytes */
> +#define F2FS_MIN_LOG_SECTOR_SIZE 9
> +
> +/* support only 4KB block */
> +#define F2FS_BLK_BITS 12
> +#define F2FS_BLKSIZE (1 << F2FS_BLK_BITS)
Could you align the values above to the values at line below?
> +#define F2FS_BLK_SEC_BITS (F2FS_BLK_BITS - GRUB_DISK_SECTOR_BITS)
> +
> +#define VERSION_LEN 256
Ditto.
> +#define F2FS_MAX_EXTENSION 64
> +
> +#define CP_COMPACT_SUM_FLAG 0x00000004
> +#define CP_UMOUNT_FLAG 0x00000001
Ditto.
> +
> +#define MAX_ACTIVE_LOGS 16
Ditto.
> +#define MAX_ACTIVE_NODE_LOGS 8
> +#define MAX_ACTIVE_DATA_LOGS 8
> +#define NR_CURSEG_DATA_TYPE 3
> +#define NR_CURSEG_NODE_TYPE 3
> +#define NR_CURSEG_TYPE (NR_CURSEG_DATA_TYPE + NR_CURSEG_NODE_TYPE)
Ditto.
> +
> +#define ENTRIES_IN_SUM 512
> +#define SUMMARY_SIZE 7
> +#define SUM_FOOTER_SIZE 5
> +#define JENTRY_SIZE (sizeof(struct grub_f2fs_nat_jent))
Same for 4 lines above.
> +#define SUM_ENTRIES_SIZE (SUMMARY_SIZE * ENTRIES_IN_SUM)
> +#define SUM_JOURNAL_SIZE (F2FS_BLKSIZE - SUM_FOOTER_SIZE -\
Please add space after "-" before "\".
> + SUM_ENTRIES_SIZE)
> +#define NAT_JOURNAL_ENTRIES ((SUM_JOURNAL_SIZE - 2) / JENTRY_SIZE)
> +#define NAT_JOURNAL_RESERVED ((SUM_JOURNAL_SIZE - 2) % JENTRY_SIZE)
> +
> +#define NAT_ENTRY_SIZE (sizeof(struct grub_f2fs_nat_entry))
> +#define NAT_ENTRY_PER_BLOCK (F2FS_BLKSIZE / NAT_ENTRY_SIZE)
Lack of alignment for two lines above.
> +#define F2FS_NAME_LEN 255
> +#define F2FS_SLOT_LEN 8
> +#define NR_DENTRY_IN_BLOCK 214
> +#define SIZE_OF_DIR_ENTRY 11 /* by byte */
> +#define BITS_PER_BYTE 8
More unaligned lines.
> +#define SIZE_OF_DENTRY_BITMAP ((NR_DENTRY_IN_BLOCK + BITS_PER_BYTE -
> 1) / \
> + BITS_PER_BYTE)
> +#define SIZE_OF_RESERVED (F2FS_BLKSIZE - ((SIZE_OF_DIR_ENTRY + \
> + F2FS_SLOT_LEN) * \
> + NR_DENTRY_IN_BLOCK + SIZE_OF_DENTRY_BITMAP))
> +
> +#define F2FS_INLINE_XATTR_ADDRS 50 /* 200 bytes for inline xattrs
> */
> +#define DEF_ADDRS_PER_INODE 923 /* Address Pointers in an Inode */
> +
> +#define ADDRS_PER_BLOCK 1018 /* Address Pointers in a Direct Block */
> +#define NIDS_PER_BLOCK 1018 /* Node IDs in an Indirect Block */
> +#define NODE_DIR1_BLOCK (DEF_ADDRS_PER_INODE + 1)
> +#define NODE_DIR2_BLOCK (DEF_ADDRS_PER_INODE + 2)
> +#define NODE_IND1_BLOCK (DEF_ADDRS_PER_INODE + 3)
> +#define NODE_IND2_BLOCK (DEF_ADDRS_PER_INODE + 4)
> +#define NODE_DIND_BLOCK (DEF_ADDRS_PER_INODE + 5)
Same as above...
> +#define MAX_INLINE_DATA (4 * (DEF_ADDRS_PER_INODE - \
> + F2FS_INLINE_XATTR_ADDRS - 1))
> +#define NR_INLINE_DENTRY (MAX_INLINE_DATA * BITS_PER_BYTE / \
> + ((SIZE_OF_DIR_ENTRY + F2FS_SLOT_LEN) * \
> + BITS_PER_BYTE + 1))
> +#define INLINE_DENTRY_BITMAP_SIZE ((NR_INLINE_DENTRY + \
> + BITS_PER_BYTE - 1) / BITS_PER_BYTE)
> +#define INLINE_RESERVED_SIZE (MAX_INLINE_DATA - \
> + ((SIZE_OF_DIR_ENTRY + F2FS_SLOT_LEN) * \
> + NR_INLINE_DENTRY + INLINE_DENTRY_BITMAP_SIZE))
> +#define CURSEG_HOT_DATA 0
Unreadable mess, please fix this...
> +#define CKPT_FLAG_SET(ckpt, f) \
> + (ckpt)->ckpt_flags & grub_cpu_to_le32_compile_time (f)
> +
> +#define F2FS_INLINE_XATTR 0x01 /* file inline xattr flag */
> +#define F2FS_INLINE_DATA 0x02 /* file inline data flag */
> +#define F2FS_INLINE_DENTRY 0x04 /* file inline dentry flag */
> +#define F2FS_DATA_EXIST 0x08 /* file inline data exist flag
> */
> +#define F2FS_INLINE_DOTS 0x10 /* file having implicit dot dentries */
> +
> +enum FILE_TYPE
> +{
> + F2FS_FT_UNKNOWN,
> + F2FS_FT_REG_FILE = 1,
> + F2FS_FT_DIR = 2,
> + F2FS_FT_SYMLINK = 7,
Could you do this?
F2FS_FT_REG_FILE = 1,
F2FS_FT_DIR = 2,
F2FS_FT_SYMLINK = 7,
> +};
> +
> +#define MAX_VOLUME_NAME 512
Please put this together with constants definitions.
> +struct grub_f2fs_superblock
> +{
> + grub_uint32_t magic;
> + grub_uint16_t dummy1[2];
> + grub_uint32_t log_sectorsize;
> + grub_uint32_t log_sectors_per_block;
> + grub_uint32_t log_blocksize;
> + grub_uint32_t log_blocks_per_seg;
> + grub_uint32_t segs_per_sec;
> + grub_uint32_t secs_per_zone;
> + grub_uint32_t checksum_offset;
> + grub_uint8_t dummy2[40];
> + grub_uint32_t cp_blkaddr;
> + grub_uint32_t sit_blkaddr;
> + grub_uint32_t nat_blkaddr;
> + grub_uint32_t ssa_blkaddr;
> + grub_uint32_t main_blkaddr;
> + grub_uint32_t root_ino;
> + grub_uint32_t node_ino;
> + grub_uint32_t meta_ino;
> + grub_uint8_t uuid[16];
> + grub_uint16_t volume_name[MAX_VOLUME_NAME];
> + grub_uint32_t extension_count;
> + grub_uint8_t extension_list[F2FS_MAX_EXTENSION][8];
> + grub_uint32_t cp_payload;
> + grub_uint8_t version[VERSION_LEN];
> + grub_uint8_t init_version[VERSION_LEN];
Could you align all member names in one column?
Please use spaces here, e.g. dummy2[] requires 2 spaces.
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_checkpoint
> +{
> + grub_uint64_t checkpoint_ver;
> + grub_uint64_t user_block_count;
> + grub_uint64_t valid_block_count;
> + grub_uint32_t rsvd_segment_count;
> + grub_uint32_t overprov_segment_count;
> + grub_uint32_t free_segment_count;
> + grub_uint32_t cur_node_segno[MAX_ACTIVE_NODE_LOGS];
> + grub_uint16_t cur_node_blkoff[MAX_ACTIVE_NODE_LOGS];
> + grub_uint32_t cur_data_segno[MAX_ACTIVE_DATA_LOGS];
> + grub_uint16_t cur_data_blkoff[MAX_ACTIVE_DATA_LOGS];
> + grub_uint32_t ckpt_flags;
> + grub_uint32_t cp_pack_total_block_count;
> + grub_uint32_t cp_pack_start_sum;
> + grub_uint32_t valid_node_count;
> + grub_uint32_t valid_inode_count;
> + grub_uint32_t next_free_nid;
> + grub_uint32_t sit_ver_bitmap_bytesize;
> + grub_uint32_t nat_ver_bitmap_bytesize;
> + grub_uint32_t checksum_offset;
> + grub_uint64_t elapsed_time;
> + grub_uint8_t alloc_type[MAX_ACTIVE_LOGS];
> + grub_uint8_t sit_nat_version_bitmap[3900];
Ditto.
> + grub_uint32_t checksum;
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_nat_entry {
> + grub_uint8_t version;
Same as above.
> + grub_uint32_t ino;
> + grub_uint32_t block_addr;
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_nat_jent
> +{
> + grub_uint32_t nid;
> + struct grub_f2fs_nat_entry ne;
Ditto. Hmmm... I am not sure about this one...
If there are structs in struct then leave them as is.
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_nat_journal {
> + grub_uint16_t n_nats;
> + struct grub_f2fs_nat_jent entries[NAT_JOURNAL_ENTRIES];
> + grub_uint8_t reserved[NAT_JOURNAL_RESERVED];
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_nat_block {
> + struct grub_f2fs_nat_entry ne[NAT_ENTRY_PER_BLOCK];
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_dir_entry
> +{
> + grub_uint32_t hash_code;
> + grub_uint32_t ino;
> + grub_uint16_t name_len;
> + grub_uint8_t file_type;
However, align this please.
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_inline_dentry
> +{
> + grub_uint8_t dentry_bitmap[INLINE_DENTRY_BITMAP_SIZE];
> + grub_uint8_t reserved[INLINE_RESERVED_SIZE];
> + struct grub_f2fs_dir_entry dentry[NR_INLINE_DENTRY];
> + grub_uint8_t filename[NR_INLINE_DENTRY][F2FS_SLOT_LEN];
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_dentry_block {
> + grub_uint8_t dentry_bitmap[SIZE_OF_DENTRY_BITMAP];
> + grub_uint8_t reserved[SIZE_OF_RESERVED];
> + struct grub_f2fs_dir_entry dentry[NR_DENTRY_IN_BLOCK];
> + grub_uint8_t filename[NR_DENTRY_IN_BLOCK][F2FS_SLOT_LEN];
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_inode
> +{
> + grub_uint16_t i_mode;
> + grub_uint8_t i_advise;
> + grub_uint8_t i_inline;
> + grub_uint32_t i_uid;
> + grub_uint32_t i_gid;
> + grub_uint32_t i_links;
> + grub_uint64_t i_size;
> + grub_uint64_t i_blocks;
> + grub_uint64_t i_atime;
> + grub_uint64_t i_ctime;
> + grub_uint64_t i_mtime;
> + grub_uint32_t i_atime_nsec;
> + grub_uint32_t i_ctime_nsec;
> + grub_uint32_t i_mtime_nsec;
> + grub_uint32_t i_generation;
> + grub_uint32_t i_current_depth;
> + grub_uint32_t i_xattr_nid;
> + grub_uint32_t i_flags;
> + grub_uint32_t i_pino;
> + grub_uint32_t i_namelen;
> + grub_uint8_t i_name[F2FS_NAME_LEN];
> + grub_uint8_t i_dir_level;
> + grub_uint8_t i_ext[12];
> + grub_uint32_t i_addr[DEF_ADDRS_PER_INODE];
> + grub_uint32_t i_nid[5];
Ditto.
> +} GRUB_PACKED;
> +
> +struct grub_direct_node {
> + grub_uint32_t addr[ADDRS_PER_BLOCK];
> +} GRUB_PACKED;
> +
> +struct grub_indirect_node {
> + grub_uint32_t nid[NIDS_PER_BLOCK];
> +} GRUB_PACKED;
> +
> +struct grub_f2fs_node
> +{
> + union
> + {
> + struct grub_f2fs_inode i;
> + struct grub_direct_node dn;
> + struct grub_indirect_node in;
> + char buf[F2FS_BLKSIZE - 40]; /* Should occupy F2FS_BLKSIZE totally */
Could you move the comment above this line?
> + };
> + grub_uint8_t dummy[40];
> +} GRUB_PACKED;
[...]
> +static inline int
Do we really need to enforce inlining here and below? I think that
compiler should do work for us.
> +grub_f2fs_test_bit_le (int nr, const grub_uint8_t *addr)
> +{
> + return addr[nr >> 3] & (1 << (nr & 7));
> +}
> +
> +static inline char *
> +__inline_addr (struct grub_f2fs_inode *inode)
> +{
> + return (char *)&inode->i_addr[1];
> +}
> +
> +static inline grub_uint64_t
> +grub_f2fs_file_size (struct grub_f2fs_inode *inode)
> +{
> + return grub_le_to_cpu64 (inode->i_size);
> +}
> +
> +static inline grub_uint32_t
I am not sure about this one... Could you double check it?
> +__start_cp_addr (struct grub_f2fs_data *data)
> +{
> + struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> + grub_uint32_t start_addr = data->cp_blkaddr;
> +
> + if (!(ckpt->checkpoint_ver & grub_cpu_to_le64_compile_time(1)))
> + return start_addr + data->blocks_per_seg;
> + return start_addr;
> +}
> +
> +static inline grub_uint32_t
> +__start_sum_block (struct grub_f2fs_data *data)
> +{
> + struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> +
> + return __start_cp_addr (data) + grub_le_to_cpu32 (ckpt->cp_pack_start_sum);
> +}
> +
> +static inline grub_uint32_t
> +__sum_blk_addr (struct grub_f2fs_data *data, int base, int type)
> +{
> + struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> +
> + return __start_cp_addr (data) +
> + grub_le_to_cpu32 (ckpt->cp_pack_total_block_count)
> + - (base + 1) + type;
> +}
> +
> +static inline void *
> +__nat_bitmap_ptr (struct grub_f2fs_data *data)
> +{
> + struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> + grub_uint32_t offset;
> +
> + if (grub_le_to_cpu32 (data->sblock.cp_payload) > 0)
> + return ckpt->sit_nat_version_bitmap;
> +
> + offset = grub_le_to_cpu32 (ckpt->sit_ver_bitmap_bytesize);
> + return ckpt->sit_nat_version_bitmap + offset;
> +}
> +
> +static inline grub_uint32_t
> +__get_node_id (struct grub_f2fs_node *rn, int off, int inode_block)
> +{
> + if (inode_block)
> + return grub_le_to_cpu32 (rn->i.i_nid[off - NODE_DIR1_BLOCK]);
> + return grub_le_to_cpu32 (rn->in.nid[off]);
> +}
> +
> +static inline grub_err_t
> +grub_f2fs_block_read (struct grub_f2fs_data *data, grub_uint32_t blkaddr,
> void *buf)
> +{
> + return grub_disk_read (data->disk,
> + ((grub_disk_addr_t)blkaddr) << F2FS_BLK_SEC_BITS,
> + 0, F2FS_BLKSIZE, buf);
> +}
> +
> +/*
> + * CRC32
> +*/
> +#define CRCPOLY_LE 0xedb88320
Please move this to constants definitions.
[...]
> +static void *
> +validate_checkpoint (struct grub_f2fs_data *data, grub_uint32_t cp_addr,
> + grub_uint64_t *version)
> +{
> + grub_uint32_t *cp_page_1, *cp_page_2;
> + struct grub_f2fs_checkpoint *cp_block;
> + grub_uint64_t cur_version = 0, pre_version = 0;
> + grub_uint32_t crc = 0;
> + grub_uint32_t crc_offset;
> + grub_err_t err;
> +
> + /* Read the 1st cp block in this CP pack */
> + cp_page_1 = grub_malloc (F2FS_BLKSIZE);
> + if (!cp_page_1)
> + return NULL;
> +
> + err = grub_f2fs_block_read (data, cp_addr, cp_page_1);
> + if (err)
> + goto invalid_cp1;
> +
> + cp_block = (struct grub_f2fs_checkpoint *)cp_page_1;
> + crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> + if (crc_offset != CHECKSUM_OFFSET)
> + goto invalid_cp1;
> +
> + crc = grub_le_to_cpu32 (*(cp_page_1 + U32_CHECKSUM_OFFSET));
> + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> + goto invalid_cp1;
> +
> + pre_version = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> +
> + /* Read the 2nd cp block in this CP pack */
> + cp_page_2 = grub_malloc (F2FS_BLKSIZE);
> + if (!cp_page_2)
> + goto invalid_cp1;
> +
> + cp_addr += grub_le_to_cpu32 (cp_block->cp_pack_total_block_count) - 1;
> +
> + err = grub_f2fs_block_read (data, cp_addr, cp_page_2);
> + if (err)
> + goto invalid_cp2;
> +
> + cp_block = (struct grub_f2fs_checkpoint *)cp_page_2;
> + crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> + if (crc_offset != CHECKSUM_OFFSET)
> + goto invalid_cp2;
> +
> + crc = grub_le_to_cpu32 (*(cp_page_2 + U32_CHECKSUM_OFFSET));
> + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> + goto invalid_cp2;
> +
> + cur_version = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> + if (cur_version == pre_version)
> + {
> + *version = cur_version;
> + grub_free (cp_page_2);
> + return cp_page_1;
> + }
> +
> +invalid_cp2:
Please put one space before each label...
> + grub_free (cp_page_2);
...and empty line before each label.
> +invalid_cp1:
> + grub_free (cp_page_1);
> + return NULL;
> +}
> +
> +static grub_err_t
> +grub_f2fs_read_cp (struct grub_f2fs_data *data)
> +{
> + void *cp1, *cp2, *cur_page;
> + grub_uint64_t cp1_version = 0, cp2_version = 0;
> + grub_uint64_t cp_start_blk_no;
> +
> + /*
> + * Finding out valid cp block involves read both
> + * sets (cp pack1 and cp pack 2)
> + */
> + cp_start_blk_no = data->cp_blkaddr;
> + cp1 = validate_checkpoint (data, cp_start_blk_no, &cp1_version);
> + if (!cp1 && grub_errno)
> + return grub_errno;
> +
> + /* The second checkpoint pack should start at the next segment */
> + cp_start_blk_no += data->blocks_per_seg;
> + cp2 = validate_checkpoint (data, cp_start_blk_no, &cp2_version);
> + if (!cp2 && grub_errno)
> + {
> + grub_free (cp1);
> + return grub_errno;
> + }
> +
> + if (cp1 && cp2)
> + cur_page = (cp2_version > cp1_version) ? cp2 : cp1;
> + else if (cp1)
> + cur_page = cp1;
> + else if (cp2)
> + cur_page = cp2;
> + else
> + return grub_error (GRUB_ERR_BAD_FS, "no checkpoints");
> +
> + grub_memcpy (&data->ckpt, cur_page, F2FS_BLKSIZE);
> +
> + grub_free (cp1);
> + grub_free (cp2);
> + return 0;
> +}
> +
> +static grub_err_t
> +get_nat_journal (struct grub_f2fs_data *data)
> +{
> + grub_uint32_t block;
> + char *buf;
> + grub_err_t err;
> +
> + buf = grub_malloc (F2FS_BLKSIZE);
> + if (!buf)
> + return grub_errno;
> +
> + if (CKPT_FLAG_SET(&data->ckpt, CP_COMPACT_SUM_FLAG))
> + block = __start_sum_block (data);
> + else if (CKPT_FLAG_SET (&data->ckpt, CP_UMOUNT_FLAG))
> + block = __sum_blk_addr (data, NR_CURSEG_TYPE, CURSEG_HOT_DATA);
> + else
> + block = __sum_blk_addr (data, NR_CURSEG_DATA_TYPE, CURSEG_HOT_DATA);
> +
> + err = grub_f2fs_block_read (data, block, buf);
> + if (err)
> + goto fail;
> +
> + if (CKPT_FLAG_SET (&data->ckpt, CP_COMPACT_SUM_FLAG))
> + grub_memcpy (&data->nat_j, buf, SUM_JOURNAL_SIZE);
> + else
> + grub_memcpy (&data->nat_j, buf + SUM_ENTRIES_SIZE, SUM_JOURNAL_SIZE);
> +
> +fail:
Ditto and below...
> + grub_free (buf);
> + return err;
> +}
Daniel