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.)