I noticed that the two cgload calls are different:

  e2fs_cgload(bp->b_data,
                    &fs->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, &m_fs->e2fs_gd[i * sh],
                                                                    ^^^^^
                    m_fs->e2fs_bsize, m_fs->e2fs_group_desc_shift);

e2fs_cgsave(&fs->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:
        &fs->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 
> <phco...@gmail.com> wrote:
> 
> 
> 
> 
> Le dim. 27 août 2023, 00:48, Christos Zoulas <chris...@zoulas.com 
> <mailto:chris...@zoulas.com>> 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 <chris...@zoulas.com 
>>> <mailto:chris...@zoulas.com>> 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 <mailto: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 
>>>> <mailto:campbell%2bnetbsd-tech-k...@mumble.net>> a écrit :
>>>> 
>>>>> > Date: Wed, 23 Aug 2023 04:54:34 +0200
>>>>> > From: "Vladimir 'phcoder' Serbinenko" <phco...@gmail.com 
>>>>> > <mailto:phco...@gmail.com>>
>>>>> >
>>>>> > 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.0000000] 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.)
>>>> 
>>>> <sanitizer.log>
>>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>> 
>> 
>> 
>> 
> 
> 
> 
> <sanitizer.log>

Attachment: signature.asc
Description: Message signed with OpenPGP

Reply via email to