lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sun, 15 Jan 2012 10:25:57 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	Amir Goldstein <amir73il@...il.com>
Cc:	Andreas Dilger <adilger@...ger.ca>, Theodore Ts'o <tytso@....edu>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 1/3] ext4: remove block bitmap initialization in ext4_new_inode()

On 2012-01-14, at 11:41, Amir Goldstein <amir73il@...il.com> wrote:

> On Sat, Jan 14, 2012 at 7:02 AM, Andreas Dilger <adilger@...ger.ca> wrote:
>> 
>> On 2012-01-13, at 2:30 PM, Theodore Ts'o wrote:
>>> We don't need to initialize the block bitmap when we allocate a new
>>> inode.
>> 
>> The reason the block bitmap is initialized when an inode is allocated
>> in that group is to indicate that the inode bitmap is in use, and the
>> inode table blocks are also in use.
> 
> but the inode table blocks will not be from this block group when flex_bg
> is used and if it's the first group of a flex_bg group, then it's bitmaps
> are already initialized by mkfs/resize.

In that case the code could detect that  the bitmap covering the inode bitmap and inode table is already initialized and not do anything. 

>>> This is old code from the very early days that is just
>>> confusing things, and also has the problem of modifying the block
>>> group descriptor without obeying the ext4_journal_get_write_access() /
>>> ext4_handle_dirty_metadata() modification protocols.
>> 
>> The group descriptor was already modified earlier on when the inode was
>> being allocated from the group, so the group descriptor block was already
>> accounted for in the transaction credits after "repeat_in_this_group:"
>> 
>> repeat_in_this_group:
>>                ino = ext4_find_next_zero_bit((unsigned long *)
>>                                              inode_bitmap_bh->b_data,
>>                                              EXT4_INODES_PER_GROUP(sb), ino);
>> 
>>                if (ino < EXT4_INODES_PER_GROUP(sb)) {
>> 
>>                        BUFFER_TRACE(inode_bitmap_bh, "get_write_access");
>>                        err = ext4_journal_get_write_access(handle,
>>                                                            inode_bitmap_bh);
>>                        if (err)
>>                                goto fail;
>> 
>>                        BUFFER_TRACE(group_desc_bh, "get_write_access");
>> *****>>>>>              err = ext4_journal_get_write_access(handle,
>>                                                            group_desc_bh);
>> 
>> That is why ext4_handle_dirty_metadata() isn't called until after the group
>> descriptor is modified during the block bitmap initialization.
>> 
>> 
>> I'm not wholly against removing this code, but we have to do it with the
>> clear understanding that we will have inodes in use for which the block
>> bitmap is showing that the in-use blocks are free.  This doesn't seem like
>> a great situation to me.
>> 
>> 
>>> Signed-off-by: "Theodore Ts'o" <tytso@....edu>
>>> ---
>>> fs/ext4/ialloc.c |   31 -------------------------------
>>> 1 files changed, 0 insertions(+), 31 deletions(-)
>>> 
>>> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
>>> index 72fc989..a4ce10f 100644
>>> --- a/fs/ext4/ialloc.c
>>> +++ b/fs/ext4/ialloc.c
>>> @@ -807,37 +807,6 @@ repeat_in_this_group:
>>>       goto out;
>>> 
>>> got:
>>> -     /* We may have to initialize the block bitmap if it isn't already */
>>> -     if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
>>> -         gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
>>> -             struct buffer_head *block_bitmap_bh;
>>> -
>>> -             block_bitmap_bh = ext4_read_block_bitmap(sb, group);
>>> -             BUFFER_TRACE(block_bitmap_bh, "get block bitmap access");
>>> -             err = ext4_journal_get_write_access(handle, block_bitmap_bh);
>>> -             if (err) {
>>> -                     brelse(block_bitmap_bh);
>>> -                     goto fail;
>>> -             }
>>> -
>>> -             BUFFER_TRACE(block_bitmap_bh, "dirty block bitmap");
>>> -             err = ext4_handle_dirty_metadata(handle, NULL, block_bitmap_bh);
>>> -             brelse(block_bitmap_bh);
>>> -
>>> -             /* recheck and clear flag under lock if we still need to */
>>> -             ext4_lock_group(sb, group);
>>> -             if (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
>>> -                     gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT);
>>> -                     ext4_free_group_clusters_set(sb, gdp,
>>> -                             ext4_free_clusters_after_init(sb, group, gdp));
>>> -                     gdp->bg_checksum = ext4_group_desc_csum(sbi, group,
>>> -                                                             gdp);
>>> -             }
>>> -             ext4_unlock_group(sb, group);
>>> -
>>> -             if (err)
>>> -                     goto fail;
>>> -     }
>>>       BUFFER_TRACE(group_desc_bh, "call ext4_handle_dirty_metadata");
>>>       err = ext4_handle_dirty_metadata(handle, NULL, group_desc_bh);
>>>       if (err)
>>> --
>>> 1.7.8.11.gefc1f.dirty
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> 
>> Cheers, Andreas
>> 
>> 
>> 
>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists