Re: [PATCH] Support INCOMPAT_64BIT on ext4

[Christos Zoulas]

I noticed that the two cgload calls are different:

  e2fs_cgload(bp->b_data,
>e2fs_gd[i * fs->e2fs_bsize / sizeof(struct ext2_gd)],
fs->e2fs_bsize, 1 << fs->e2fs_group_desc_shift);
  ^
int32_t sh = m_fs->e2fs_bsize >> m_fs->e2fs_group_desc_shift;
e2fs_cgload(bp->b_data, _fs->e2fs_gd[i * sh],
^
m_fs->e2fs_bsize, m_fs->e2fs_group_desc_shift);

e2fs_cgsave(>e2fs_gd[
i * fs->e2fs_bsize / sizeof(struct ext2_gd)],
bp->b_data, fs->e2fs_bsize, fs->e2fs_group_desc_shift);

shouldn't we consistently index as:
>e2fs_gd[i * fs->e2fs_bsize / sizeof(struct ext2_gd)]
and pass the shift as:
fs->e2fs_group_desc_shift

???

Thanks,

christos

> On Aug 26, 2023, at 7:43 PM, Vladimir 'phcoder' Serbinenko 
>  wrote:
> 
> 
> 
> 
> Le dim. 27 août 2023, 00:48, Christos Zoulas  > a écrit :
> 
>> I took care of it. One I think was mine, one was yours  :-)
>> 
> 
> 
> Thank you a lot. I was diagnosing it but you were faster. I'll see what I can 
> do for userspace to remove old cgload/cgsave
> 
>> 
>> christos
>> 
>> 
>> 
>>> On Aug 26, 2023, at 8:52 AM, Christos Zoulas >> > wrote:
>>> 
>>> 
>>> It could also be my fault because I refactored the code a bit. One of the 
>>> things that I changed
>>> 
>>> that looked like a bug to me was adding a cast to e2fs_cgload():
>>> 
>>> memset((char *)optr + 32, 0, sizeof(*optr) - 32);
>>> 
>>> 
>>> 
>>> since optr is struct ext2_gd *...
>>> 
>>> 
>>> 
>>> christos
>>> 
>>> 
>>> 
>>> 
>>> 
 On Aug 26, 2023, at 7:46 AM, Vladimir 'phcoder' Serbinenko 
 mailto:phco...@gmail.com>> wrote:
 
 
 I'll have a look today or tomorrow
 
 
 Le sam. 26 août 2023, 12:06, Taylor R Campbell 
 >>> > a écrit :
 
> > Date: Wed, 23 Aug 2023 04:54:34 +0200
> > From: "Vladimir 'phcoder' Serbinenko"  > >
> >
> > This patch adds support for incompat_64bit on ext4 filesystem. This 
> > feature
> > is enabled by default on new filesystems on Ubuntu and probably other
> > distros
> 
> Cool, thanks!  christos@ committed this.
> 
> It looks like it may have some issues, though -- a lot of ext2fs tests
> are failing now.  Can you please take a look at the failures?
> 
> https://releng.netbsd.org/b5reports/i386/commits-2023.08.html#build-2023.08.26.05.47.53
> 
>Termination reason
> 
>FAILED: create file: No space left on device
> 
>Standard output stream
> 
>[   1.000] entropy: ready
>[   1.0200050] uid 0 on /mnt: out of inodes
> 
> FYI, if you're running a new enough kernel already, you can run the
> tests yourself by doing a distribution build and then doing
> 
> # chroot /path/to/objdir/destdir.amd64
> (chroot)# (cd /dev && sh MAKEDEV all)
> (chroot)# mount -t ptyfs ptyfs /dev/pts
> (chroot)# mount -t tmpfs tmpfs /tmp
> (chroot)# cd /usr/tests/fs/vfs
> (chroot)# atf-run | atf-report
> 
> You can also do build.sh release, install into a VM, and run the tests
> the same way -- cd /usr/tests/fs/vfs && atf-run | atf-report.
> 
> This also broke the build of bootloaders and the newfs_ext2fs userland
> tool.  I put in a stop-gap measure to restore the old definitions of
> e2fs_cgload/cgsave outside the kernel, just to unbreak the build, but
> it might be appropriate to make the new definitions available to
> bootloaders and newfs_ext2fs too.
> 
> (I haven't looked into technical details to see whether it is
> appropriate.  Probably newfs_ext2s should avoid creating file systems
> this new feature for a while, but maybe have an option to do it so we
> can exercise the code paths in tests.)
 
 
 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>> 
>> 
>> 
>> 
> 
> 
> 
> 



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH] Support INCOMPAT_64BIT on ext4

[Vladimir 'phcoder' Serbinenko]

Le dim. 27 août 2023, 00:48, Christos Zoulas  a écrit :

> I took care of it. One I think was mine, one was yours  :-)
>
Thank you a lot. I was diagnosing it but you were faster. I'll see what I
can do for userspace to remove old cgload/cgsave

>
> christos
>
> On Aug 26, 2023, at 8:52 AM, Christos Zoulas  wrote:
>
> It could also be my fault because I refactored the code a bit. One of the
> things that I changed
> that looked like a bug to me was adding a cast to e2fs_cgload():
> memset((char *)optr + 32, 0, sizeof(*optr) - 32);
>
> since optr is struct ext2_gd *...
>
> christos
>
>
> On Aug 26, 2023, at 7:46 AM, Vladimir 'phcoder' Serbinenko <
> phco...@gmail.com> wrote:
>
> I'll have a look today or tomorrow
>
> Le sam. 26 août 2023, 12:06, Taylor R Campbell <
> campbell+netbsd-tech-k...@mumble.net> a écrit :
>
>> > Date: Wed, 23 Aug 2023 04:54:34 +0200
>> > From: "Vladimir 'phcoder' Serbinenko" 
>> >
>> > This patch adds support for incompat_64bit on ext4 filesystem. This
>> feature
>> > is enabled by default on new filesystems on Ubuntu and probably other
>> > distros
>>
>> Cool, thanks!  christos@ committed this.
>>
>> It looks like it may have some issues, though -- a lot of ext2fs tests
>> are failing now.  Can you please take a look at the failures?
>>
>>
>> https://releng.netbsd.org/b5reports/i386/commits-2023.08.html#build-2023.08.26.05.47.53
>>
>>Termination reason
>>
>>FAILED: create file: No space left on device
>>
>>Standard output stream
>>
>>[   1.000] entropy: ready
>>[   1.0200050] uid 0 on /mnt: out of inodes
>>
>> FYI, if you're running a new enough kernel already, you can run the
>> tests yourself by doing a distribution build and then doing
>>
>> # chroot /path/to/objdir/destdir.amd64
>> (chroot)# (cd /dev && sh MAKEDEV all)
>> (chroot)# mount -t ptyfs ptyfs /dev/pts
>> (chroot)# mount -t tmpfs tmpfs /tmp
>> (chroot)# cd /usr/tests/fs/vfs
>> (chroot)# atf-run | atf-report
>>
>> You can also do build.sh release, install into a VM, and run the tests
>> the same way -- cd /usr/tests/fs/vfs && atf-run | atf-report.
>>
>> This also broke the build of bootloaders and the newfs_ext2fs userland
>> tool.  I put in a stop-gap measure to restore the old definitions of
>> e2fs_cgload/cgsave outside the kernel, just to unbreak the build, but
>> it might be appropriate to make the new definitions available to
>> bootloaders and newfs_ext2fs too.
>>
>> (I haven't looked into technical details to see whether it is
>> appropriate.  Probably newfs_ext2s should avoid creating file systems
>> this new feature for a while, but maybe have an option to do it so we
>> can exercise the code paths in tests.)
>>
>
> 
>
>
>
>

Re: [PATCH] Support INCOMPAT_64BIT on ext4

[Christos Zoulas]

I took care of it. One I think was mine, one was yours  :-)

christos

> On Aug 26, 2023, at 8:52 AM, Christos Zoulas  wrote:
> 
> It could also be my fault because I refactored the code a bit. One of the 
> things that I changed
> that looked like a bug to me was adding a cast to e2fs_cgload():
>   memset((char *)optr + 32, 0, sizeof(*optr) - 32);
> 
> since optr is struct ext2_gd *...
> 
> christos
> 
> 
>> On Aug 26, 2023, at 7:46 AM, Vladimir 'phcoder' Serbinenko 
>>  wrote:
>> 
>> I'll have a look today or tomorrow
>> 
>> 
>> Le sam. 26 août 2023, 12:06, Taylor R Campbell 
>> > > a écrit :
>> 
>>> > Date: Wed, 23 Aug 2023 04:54:34 +0200
>>> > From: "Vladimir 'phcoder' Serbinenko" >> > >
>>> >
>>> > This patch adds support for incompat_64bit on ext4 filesystem. This 
>>> > feature
>>> > is enabled by default on new filesystems on Ubuntu and probably other
>>> > distros
>>> 
>>> Cool, thanks!  christos@ committed this.
>>> 
>>> It looks like it may have some issues, though -- a lot of ext2fs tests
>>> are failing now.  Can you please take a look at the failures?
>>> 
>>> https://releng.netbsd.org/b5reports/i386/commits-2023.08.html#build-2023.08.26.05.47.53
>>> 
>>>Termination reason
>>> 
>>>FAILED: create file: No space left on device
>>> 
>>>Standard output stream
>>> 
>>>[   1.000] entropy: ready
>>>[   1.0200050] uid 0 on /mnt: out of inodes
>>> 
>>> FYI, if you're running a new enough kernel already, you can run the
>>> tests yourself by doing a distribution build and then doing
>>> 
>>> # chroot /path/to/objdir/destdir.amd64
>>> (chroot)# (cd /dev && sh MAKEDEV all)
>>> (chroot)# mount -t ptyfs ptyfs /dev/pts
>>> (chroot)# mount -t tmpfs tmpfs /tmp
>>> (chroot)# cd /usr/tests/fs/vfs
>>> (chroot)# atf-run | atf-report
>>> 
>>> You can also do build.sh release, install into a VM, and run the tests
>>> the same way -- cd /usr/tests/fs/vfs && atf-run | atf-report.
>>> 
>>> This also broke the build of bootloaders and the newfs_ext2fs userland
>>> tool.  I put in a stop-gap measure to restore the old definitions of
>>> e2fs_cgload/cgsave outside the kernel, just to unbreak the build, but
>>> it might be appropriate to make the new definitions available to
>>> bootloaders and newfs_ext2fs too.
>>> 
>>> (I haven't looked into technical details to see whether it is
>>> appropriate.  Probably newfs_ext2s should avoid creating file systems
>>> this new feature for a while, but maybe have an option to do it so we
>>> can exercise the code paths in tests.)
>> 
>> 
> 



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH] Support INCOMPAT_64BIT on ext4

[Vladimir 'phcoder' Serbinenko]

I'll have a look today or tomorrow

Le sam. 26 août 2023, 12:06, Taylor R Campbell <
campbell+netbsd-tech-k...@mumble.net> a écrit :

> > Date: Wed, 23 Aug 2023 04:54:34 +0200
> > From: "Vladimir 'phcoder' Serbinenko" 
> >
> > This patch adds support for incompat_64bit on ext4 filesystem. This
> feature
> > is enabled by default on new filesystems on Ubuntu and probably other
> > distros
>
> Cool, thanks!  christos@ committed this.
>
> It looks like it may have some issues, though -- a lot of ext2fs tests
> are failing now.  Can you please take a look at the failures?
>
>
> https://releng.netbsd.org/b5reports/i386/commits-2023.08.html#build-2023.08.26.05.47.53
>
>Termination reason
>
>FAILED: create file: No space left on device
>
>Standard output stream
>
>[   1.000] entropy: ready
>[   1.0200050] uid 0 on /mnt: out of inodes
>
> FYI, if you're running a new enough kernel already, you can run the
> tests yourself by doing a distribution build and then doing
>
> # chroot /path/to/objdir/destdir.amd64
> (chroot)# (cd /dev && sh MAKEDEV all)
> (chroot)# mount -t ptyfs ptyfs /dev/pts
> (chroot)# mount -t tmpfs tmpfs /tmp
> (chroot)# cd /usr/tests/fs/vfs
> (chroot)# atf-run | atf-report
>
> You can also do build.sh release, install into a VM, and run the tests
> the same way -- cd /usr/tests/fs/vfs && atf-run | atf-report.
>
> This also broke the build of bootloaders and the newfs_ext2fs userland
> tool.  I put in a stop-gap measure to restore the old definitions of
> e2fs_cgload/cgsave outside the kernel, just to unbreak the build, but
> it might be appropriate to make the new definitions available to
> bootloaders and newfs_ext2fs too.
>
> (I haven't looked into technical details to see whether it is
> appropriate.  Probably newfs_ext2s should avoid creating file systems
> this new feature for a while, but maybe have an option to do it so we
> can exercise the code paths in tests.)
>

Re: [PATCH] Support INCOMPAT_64BIT on ext4

[Taylor R Campbell]

> Date: Wed, 23 Aug 2023 04:54:34 +0200
> From: "Vladimir 'phcoder' Serbinenko" 
> 
> This patch adds support for incompat_64bit on ext4 filesystem. This feature
> is enabled by default on new filesystems on Ubuntu and probably other
> distros

Cool, thanks!  christos@ committed this.

It looks like it may have some issues, though -- a lot of ext2fs tests
are failing now.  Can you please take a look at the failures?

https://releng.netbsd.org/b5reports/i386/commits-2023.08.html#build-2023.08.26.05.47.53

   Termination reason

   FAILED: create file: No space left on device

   Standard output stream

   [   1.000] entropy: ready
   [   1.0200050] uid 0 on /mnt: out of inodes

FYI, if you're running a new enough kernel already, you can run the
tests yourself by doing a distribution build and then doing

# chroot /path/to/objdir/destdir.amd64
(chroot)# (cd /dev && sh MAKEDEV all)
(chroot)# mount -t ptyfs ptyfs /dev/pts
(chroot)# mount -t tmpfs tmpfs /tmp
(chroot)# cd /usr/tests/fs/vfs
(chroot)# atf-run | atf-report

You can also do build.sh release, install into a VM, and run the tests
the same way -- cd /usr/tests/fs/vfs && atf-run | atf-report.

This also broke the build of bootloaders and the newfs_ext2fs userland
tool.  I put in a stop-gap measure to restore the old definitions of
e2fs_cgload/cgsave outside the kernel, just to unbreak the build, but
it might be appropriate to make the new definitions available to
bootloaders and newfs_ext2fs too.

(I haven't looked into technical details to see whether it is
appropriate.  Probably newfs_ext2s should avoid creating file systems
this new feature for a while, but maybe have an option to do it so we
can exercise the code paths in tests.)

[PATCH] Support INCOMPAT_64BIT on ext4

[Vladimir 'phcoder' Serbinenko]

This patch adds support for incompat_64bit on ext4 filesystem. This feature
is enabled by default on new filesystems on Ubuntu and probably other
distros
From 101769076e44208c2fac45aceddd4a46c33eaefb Mon Sep 17 00:00:00 2001
From: Vladimir Serbinenko 
Date: Mon, 21 Aug 2023 20:55:35 +0200
Subject: [PATCH 1/2] ext2fs: Support INCOMPAT_64BIT feature

---
 sys/ufs/ext2fs/ext2fs.h| 27 
 sys/ufs/ext2fs/ext2fs_alloc.c  | 75 -
 sys/ufs/ext2fs/ext2fs_vfsops.c | 76 ++
 3 files changed, 132 insertions(+), 46 deletions(-)

diff --git a/sys/ufs/ext2fs/ext2fs.h b/sys/ufs/ext2fs/ext2fs.h
index 320192b03a8..ae814b76798 100644
--- a/sys/ufs/ext2fs/ext2fs.h
+++ b/sys/ufs/ext2fs/ext2fs.h
@@ -252,6 +252,7 @@ struct m_ext2fs {
 	int32_t	e2fs_ngdb;	/* number of group descriptor blocks */
 	int32_t	e2fs_ipb;	/* number of inodes per block */
 	int32_t	e2fs_itpg;	/* number of inode table blocks per group */
+	u_int8_t e2fs_group_desc_shift; /* binary log group desc size */
 	struct	ext2_gd *e2fs_gd; /* group descriptors (data not byteswapped) */
 };
 
@@ -370,7 +371,8 @@ struct m_ext2fs {
 	 | EXT2F_ROCOMPAT_GDT_CSUM)
 #define EXT2F_INCOMPAT_SUPP		(EXT2F_INCOMPAT_FTYPE \
 	 | EXT2F_INCOMPAT_EXTENTS \
-	 | EXT2F_INCOMPAT_FLEX_BG)
+	 | EXT2F_INCOMPAT_FLEX_BG \
+	 | EXT2F_INCOMPAT_64BIT)
 
 /*
  * Feature set definitions
@@ -432,10 +434,14 @@ struct ext2_gd {
 	uint16_t ext2bgd_itable_unused_lo;	/* Low unused inode offset */
 	uint16_t ext2bgd_checksum;		/* Group desc checksum */
 
-	/*
-	 * XXX disk32 Further fields only exist if 64BIT feature is on
-	 * and superblock desc_size > 32, not supported for now.
-	 */
+	u_int32_t ext2bgd_b_bitmap_hi;	/* blocks bitmap block (high bits) */
+	u_int32_t ext2bgd_i_bitmap_hi;	/* inodes bitmap block (high bits) */
+	u_int32_t ext2bgd_i_tables_hi;	/* inodes table block (high bits)  */
+	u_int16_t ext2bgd_nbfree_hi;	/* number of free blocks (high bits) */
+	u_int16_t ext2bgd_nifree_hi;	/* number of free inodes (high bits) */
+	u_int16_t ext2bgd_ndirs_hi;	/* number of directories (high bits) */
+	u_int16_t reserved_hi;
+	u_int32_t reserved2_hi[3];
 };
 
 #define E2FS_BG_INODE_UNINIT	0x0001	/* Inode bitmap not used/initialized */
@@ -492,15 +498,18 @@ void e2fs_sb_bswap(struct ext2fs *, struct ext2fs *);
 #	define e2fs_sbsave(old, new) e2fs_sb_bswap((old), (new))
 #endif
 
-/* Group descriptors are not byte swapped */
-#define e2fs_cgload(old, new, size) memcpy((new), (old), (size))
-#define e2fs_cgsave(old, new, size) memcpy((new), (old), (size))
+void e2fs_cgload(const char *ondisk, struct ext2_gd *inmemory,
+		 int shift_cg_entry_size, int cg_size);
+void e2fs_cgsave(const struct ext2_gd *inmemory, char *ondisk,
+		 int shift_cg_entry_size, int cg_size);
 
 /*
  * Turn file system block numbers into disk block addresses.
  * This maps file system blocks to device size blocks.
  */
 #define EXT2_FSBTODB(fs, b)	((b) << (fs)->e2fs_fsbtodb)
+#define EXT2_FSBTODB64(fs, b, b_hi)	(u_int64_t)(b_hi)) << 32) | (b)) << (fs)->e2fs_fsbtodb)
+#define EXT2_FSBTODB64OFF(fs, b, b_hi, off)	((u_int64_t)(b_hi)) << 32) | (b)) + (off)) << (fs)->e2fs_fsbtodb)
 #define EXT2_DBTOFSB(fs, b)	((b) >> (fs)->e2fs_fsbtodb)
 
 /*
@@ -512,6 +521,8 @@ void e2fs_sb_bswap(struct ext2fs *, struct ext2fs *);
 #define	ino_to_cg(fs, x)	(((x) - 1) / (fs)->e2fs.e2fs_ipg)
 #define	ino_to_fsba(fs, x)		\
 	(fs2h32((fs)->e2fs_gd[ino_to_cg((fs), (x))].ext2bgd_i_tables) +	\
+	 (((u_int64_t)fs2h32((fs)->e2fs_gd[ino_to_cg((fs), (x))].ext2bgd_i_tables_hi)) \
+	  << 32) + \
 	(((x) - 1) % (fs)->e2fs.e2fs_ipg) / (fs)->e2fs_ipb)
 #define	ino_to_fsbo(fs, x)	(((x) - 1) % (fs)->e2fs_ipb)
 
diff --git a/sys/ufs/ext2fs/ext2fs_alloc.c b/sys/ufs/ext2fs/ext2fs_alloc.c
index a130242833a..b78ba827641 100644
--- a/sys/ufs/ext2fs/ext2fs_alloc.c
+++ b/sys/ufs/ext2fs/ext2fs_alloc.c
@@ -91,7 +91,7 @@ static u_long	ext2fs_hashalloc(struct inode *, int, long, int,
 static daddr_t	ext2fs_nodealloccg(struct inode *, int, daddr_t, int);
 static daddr_t	ext2fs_mapsearch(struct m_ext2fs *, char *, daddr_t);
 static __inline void	ext2fs_cg_update(struct m_ext2fs *, int, struct ext2_gd *, int, int, int, daddr_t);
-static uint16_t 	ext2fs_cg_get_csum(struct m_ext2fs *, int, struct ext2_gd *);
+static uint16_t 	ext2fs_cg_get_csum(struct m_ext2fs *, int, struct ext2_gd *, size_t);
 static void		ext2fs_init_bb(struct m_ext2fs *, int, struct ext2_gd *, char *);
 
 /*
@@ -212,13 +212,19 @@ ext2fs_dirpref(struct m_ext2fs *fs)
 	avgifree = fs->e2fs.e2fs_ficount / fs->e2fs_ncg;
 	maxspace = 0;
 	mincg = -1;
-	for (cg = 0; cg < fs->e2fs_ncg; cg++)
-		if (fs2h16(fs->e2fs_gd[cg].ext2bgd_nifree) >= avgifree) {
-			if (mincg == -1 || fs2h16(fs->e2fs_gd[cg].ext2bgd_nbfree) > maxspace) {
+	for (cg = 0; cg < fs->e2fs_ncg; cg++) {
+		u_int32_t nifree = (fs2h16(fs->e2fs_gd[cg].ext2bgd_nifree_hi) << 16)
+			| fs2h16(fs->e2fs_gd[cg].ext2bgd_nifree);
+		if (nifree >= avgifree)