| Message ID | 20241219110027.1440876-7-shikemeng@huaweicloud.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix and cleanups to ext4 namei.c | expand |
> The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead > of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same > number either with name_len 1 or name_len 2, it's better use the right > name_len to make code more intuitive. Do you try to point a correctness issue out here? How do you think about to add any tags (like “Fixes” and “Cc”) accordingly? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145 Regards, Markus
On Dec 20, 2024, at 6:52 AM, Markus Elfring <Markus.Elfring@web.de> wrote: > >> The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead >> of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same >> number either with name_len 1 or name_len 2, it's better use the right >> name_len to make code more intuitive. > > Do you try to point a correctness issue out here? > > How do you think about to add any tags (like “Fixes” and “Cc”) accordingly? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145 The patch is a no-op in terms of functionality. Dirent lengths are rounded up to a multiple of 4 bytes, so "1" and "2" give the same value. I was looking back at the changes to this code, and it has existed since at least when ext4 was cloned from ext3. In older versions of the code this calculation was done *before* the I also realized that the original code with "1" is actually correct. This code is calculating the dirent length of the ".." entry, but this is actually "the rest of the block minus the the '.' dirent size", so passing "1" is correct in this case. So this patch should not be applied. Cheers, Andreas
on 12/21/2024 4:38 AM, Andreas Dilger wrote: > On Dec 20, 2024, at 6:52 AM, Markus Elfring <Markus.Elfring@web.de> wrote: >> >>> The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead >>> of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same >>> number either with name_len 1 or name_len 2, it's better use the right >>> name_len to make code more intuitive. >> >> Do you try to point a correctness issue out here? >> >> How do you think about to add any tags (like “Fixes” and “Cc”) accordingly? >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145 > > The patch is a no-op in terms of functionality. Dirent lengths are rounded up to a multiple of 4 bytes, so "1" and "2" give the same value. > > I was looking back at the changes to this code, and it has existed > since at least when ext4 was cloned from ext3. In older versions > of the code this calculation was done *before* the > > I also realized that the original code with "1" is actually correct. > This code is calculating the dirent length of the ".." entry, but > this is actually "the rest of the block minus the the '.' dirent size", > so passing "1" is correct in this case. > > So this patch should not be applied. Ahh, right... Thanks for point this out. Will drop this in next version. Thanks, Kemeng > > > Cheers, Andreas > > > > >
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index aee1858e6482..24cdeb2aa0d5 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2925,7 +2925,7 @@ struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode, de->name_len = 2; if (!dotdot_real_len) de->rec_len = ext4_rec_len_to_disk(blocksize - - (csum_size + ext4_dir_rec_len(1, NULL)), + (csum_size + ext4_dir_rec_len(2, NULL)), blocksize); else de->rec_len = ext4_rec_len_to_disk(
The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same number either with name_len 1 or name_len 2, it's better use the right name_len to make code more intuitive. Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- fs/ext4/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)