Re: [PATCH] F2FS support
В Sat, 28 Mar 2015 13:43:18 -0700
Jaegeuk Kim jaeg...@kernel.org пишет:
Hi Andrei,
On Sat, Mar 28, 2015 at 10:31:55AM +0300, Andrei Borzenkov wrote:
В Tue, 24 Mar 2015 01:19:00 -0700
Jaegeuk Kim jaeg...@kernel.org пишет:
* Makefile.util.def: Add f2fs.c.
* doc/grub.texi: Add f2fs description.
* grub-core/Makefile.core.def: Add f2fs module.
* grub-core/fs/f2fs.c: New file.
* tests/f2fs_test.in: New file.
* tests/util/grub-fs-tester.in: Add f2fs requirements.
It's not the most useful commit message. Better would be short
explanation of use cases and intended platforms. I'm curious here -
F2FS is intended for raw flash access, on which platform(s) grub has
access to such devices?
I just followed the commit convention in grub.git.
It has changed meanwhile. We are using normal git conventions now.
+static grub_err_t
+grub_f2fs_read_sb (struct grub_f2fs_data *data, int block)
+{
+ grub_disk_t disk = data-disk;
+ grub_uint64_t offset;
+ grub_err_t err;
+
+ if (block == 0)
+offset = F2FS_SUPER_OFFSET;
+ else
+offset = F2FS_BLKSIZE + F2FS_SUPER_OFFSET;
+
Please name it secondary or similar instead of block to avoid
confusion. You do not really want to read arbitrary block, right?
Actually it makes more sense just to pass offset directly to eliminate
useless computation.
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] F2FS support
Hi Andrei,
On Sat, Mar 28, 2015 at 10:31:55AM +0300, Andrei Borzenkov wrote:
В Tue, 24 Mar 2015 01:19:00 -0700
Jaegeuk Kim jaeg...@kernel.org пишет:
* Makefile.util.def: Add f2fs.c.
* doc/grub.texi: Add f2fs description.
* grub-core/Makefile.core.def: Add f2fs module.
* grub-core/fs/f2fs.c: New file.
* tests/f2fs_test.in: New file.
* tests/util/grub-fs-tester.in: Add f2fs requirements.
It's not the most useful commit message. Better would be short
explanation of use cases and intended platforms. I'm curious here -
F2FS is intended for raw flash access, on which platform(s) grub has
access to such devices?
I just followed the commit convention in grub.git.
Anyway I'll add some description in v2.
F2FS is *not* intended for raw flash, for general block device such as SSD,
eMMC, and SD cards.
Please refer the following documents.
http://en.wikipedia.org/wiki/F2FS
Thank you for the detailed review.
I'll fix them and send v2.
Sincerely yours,
diff --git a/ChangeLog-2015 b/ChangeLog-2015
We do not use ChangeLog any more, it is autogenerated from commits.
This file is legacy before this change, do not change it.
diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
new file mode 100644
index 000..40360d5
--- /dev/null
+++ b/grub-core/fs/f2fs.c
@@ -0,0 +1,1321 @@
+/*
+ * f2fs.c - Flash-Friendly File System
+ *
+ * Written by Jaegeuk Kim jaeg...@kernel.org
+ *
+ * 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/.
+ */
+#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
+
+/* byte-size offset */
+#define F2FS_SUPER_OFFSET 1024
+
+/* 12 bits for 4096 bytes */
+#define F2FS_MAX_LOG_SECTOR_SIZE 12
+
+/* 9 bits for 512 bytes */
+#define F2FS_MIN_LOG_SECTOR_SIZE 9
+
+/* support only 4KB block */
+#define F2FS_BLKSIZE 4096
(2 F2FS_BLK_BITS)?
+#define F2FS_BLK_BITS 12
+#define F2FS_BLK_SEC_BITS (3)
It is confusing to have some defines parenthesized and some not. Could
it be unified somehow?
Also this can be computed from F2FS_BLK_BITS and GRUB_DISK_SECTOR_BITS
- one magic number less.
...
+struct grub_f2fs_inline_dentry
+{
+ grub_uint8_t dentry_bitmap[INLINE_DENTRY_BITMAP_SIZE];
This is cast to grub_uint32_t everywhere. Can it be non-multiple of 4
bytes? If not, may be just define as such?
+ 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];
ditto
+ 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;
+
...
+
+#define ver_after (a, b) (typecheck (unsigned long long, a) \
+ typecheck (unsigned long long, b) \
+ ((long long)((a) - (b)) 0))
+
Where typecheck definition comes from?
...
+
+static inline int
+__test_bit (int nr, grub_uint32_t *addr)
+{
+ return 1UL (addr[nr / 32] (nr (31)));
Extra parenthesis (31)
+}
+
It is used for dentry_bitmap which is kept in LE format and not
converted as far as I can tell. This needs fixing for BE systems. Linux
kernel is explicitly using test_bit_le here. This will also work for
inode flags (just skip explicit conversion).
There are two functions with more or less identical names. May be make
them
grub_f2fs_test_bit_le32
grub_f2fs_test_bit
As a general comment - marking non-modified arguments as const
everywhere would be good.
+static inline char *
+__inline_addr (struct grub_f2fs_inode *inode)
+{
+ return (char *)(inode-i_addr[1]);
Redundant parens around inode-
+}
+
+static inline grub_uint64_t
+__i_size (struct grub_f2fs_inode *inode)
Could we make it
Re: [PATCH] use stock embedded timestamp 2015-01-01T0000+0000
On Fri 2015-03-27 08:27:42 -0400, Vladimir 'φ-coder/phcoder' Serbinenko wrote: @@ -1856,7 +1859,7 @@ grub_install_generate_image (const char *dir, const char *prefix, head-magic = image_target-bigendian ? grub_host_to_target16 (0x160) : grub_host_to_target16 (0x166); head-nsec = grub_host_to_target16 (1); -head-time = grub_host_to_target32 (0); +head-time = grub_host_to_target32 (STABLE_EMBEDDING_TIMESTAMP); I dropped this hunk as it's just changing one const to another. head-opt = grub_host_to_target16 (0x38); head-flags = image_target-bigendian ? grub_host_to_target16 (0x207) Sure, that's fine with me. I supplied it there so that we could say any grub image on any platform with an embedded timestamp will use the same timestamp, but if you don't think that's a necessary or useful statement to make, i have no interest in pushing it separately. My main goal is image reproducibility, and the MIPS_ARC builds were already reproducible. Thanks for adopting the fixed timestamp for the other two platforms! --dkg ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] F2FS support
В Tue, 24 Mar 2015 01:19:00 -0700
Jaegeuk Kim jaeg...@kernel.org пишет:
* Makefile.util.def: Add f2fs.c.
* doc/grub.texi: Add f2fs description.
* grub-core/Makefile.core.def: Add f2fs module.
* grub-core/fs/f2fs.c: New file.
* tests/f2fs_test.in: New file.
* tests/util/grub-fs-tester.in: Add f2fs requirements.
It's not the most useful commit message. Better would be short
explanation of use cases and intended platforms. I'm curious here -
F2FS is intended for raw flash access, on which platform(s) grub has
access to such devices?
diff --git a/ChangeLog-2015 b/ChangeLog-2015
We do not use ChangeLog any more, it is autogenerated from commits.
This file is legacy before this change, do not change it.
diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
new file mode 100644
index 000..40360d5
--- /dev/null
+++ b/grub-core/fs/f2fs.c
@@ -0,0 +1,1321 @@
+/*
+ * f2fs.c - Flash-Friendly File System
+ *
+ * Written by Jaegeuk Kim jaeg...@kernel.org
+ *
+ * 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/.
+ */
+#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
+
+/* byte-size offset */
+#define F2FS_SUPER_OFFSET1024
+
+/* 12 bits for 4096 bytes */
+#define F2FS_MAX_LOG_SECTOR_SIZE 12
+
+/* 9 bits for 512 bytes */
+#define F2FS_MIN_LOG_SECTOR_SIZE 9
+
+/* support only 4KB block */
+#define F2FS_BLKSIZE 4096
(2 F2FS_BLK_BITS)?
+#define F2FS_BLK_BITS12
+#define F2FS_BLK_SEC_BITS(3)
It is confusing to have some defines parenthesized and some not. Could
it be unified somehow?
Also this can be computed from F2FS_BLK_BITS and GRUB_DISK_SECTOR_BITS
- one magic number less.
...
+struct grub_f2fs_inline_dentry
+{
+ grub_uint8_t dentry_bitmap[INLINE_DENTRY_BITMAP_SIZE];
This is cast to grub_uint32_t everywhere. Can it be non-multiple of 4
bytes? If not, may be just define as such?
+ 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];
ditto
+ 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;
+
...
+
+#define ver_after (a, b) (typecheck (unsigned long long, a) \
+ typecheck (unsigned long long, b) \
+ ((long long)((a) - (b)) 0))
+
Where typecheck definition comes from?
...
+
+static inline int
+__test_bit (int nr, grub_uint32_t *addr)
+{
+ return 1UL (addr[nr / 32] (nr (31)));
Extra parenthesis (31)
+}
+
It is used for dentry_bitmap which is kept in LE format and not
converted as far as I can tell. This needs fixing for BE systems. Linux
kernel is explicitly using test_bit_le here. This will also work for
inode flags (just skip explicit conversion).
There are two functions with more or less identical names. May be make
them
grub_f2fs_test_bit_le32
grub_f2fs_test_bit
As a general comment - marking non-modified arguments as const
everywhere would be good.
+static inline char *
+__inline_addr (struct grub_f2fs_inode *inode)
+{
+ return (char *)(inode-i_addr[1]);
Redundant parens around inode-
+}
+
+static inline grub_uint64_t
+__i_size (struct grub_f2fs_inode *inode)
Could we make it grub_f2fs_file_size or similar? i_size really does not
tell much outside of linux kernel.
+{
+ return grub_le_to_cpu64 (inode-i_size);
+}
+
+static inline grub_uint32_t
+__start_cp_addr (struct grub_f2fs_data *data)
+{
+ struct grub_f2fs_checkpoint *ckpt = data-ckpt;
+ grub_uint64_t ckpt_version = grub_le_to_cpu64 (ckpt-checkpoint_ver);
+ grub_uint32_t start_addr = data-cp_blkaddr;
+
+ if (!(ckpt_version 1))
+return start_addr + data-blocks_per_seg;
+ return start_addr;
+}
+
+static inline grub_uint32_t
+__start_sum_block (struct
