linux-ext4 - Re: [PATCH] ext4: optimize orphan_list handling for ext4

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 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 3 Nov 2010 11:35:41 -0600
From:	Andreas Dilger <adilger.kernel@...ger.ca>
To:	Dmitry <dmonakhov@...nvz.org>
Cc:	linux-ext4@...r.kernel.org, Ted Ts'o <tytso@....edu>
Subject: Re: [PATCH] ext4: optimize orphan_list handling for ext4_setattr

On 2010-11-03, at 03:31, Dmitry wrote:
> On Wed, 3 Nov 2010 03:06:41 -0600, Andreas Dilger <adilger@...ger.ca> wrote:
>> 
>> Basically, the first hunk is moving the ext4_handle_valid() into the caller, and the second is avoiding a call to ext4_orphan_del->ext4_handle_valid(), which would likely be an unmeasurable performance difference.  No locks are taken (or avoided) before ext4_handle_valid() is checked.
> 
> But in case of nojournal mode it will be called with noop result.
> but orphan_del(NULL, inode) will result in useless locking. 

Looking at ext4_orphan_del(), I'm not sure why it checks:

        /* ext4_handle_valid() assumes a valid handle_t pointer */
        if (handle && !ext4_handle_valid(handle))
                return 0;

when in ext4_orphan_add() it is only checks:

        if (!ext4_handle_valid(handle))
                return 0;

I think the extra check for "handle" in ext4_orphan_del() is not needed, despite the comment there, because none of the other callsites for ext4_handle_valid() do this extra check.

>> Even better would be a no-lock check of list_empty(&ei->i_orphan()) before getting s_orphan_lock, since it shouldn't be possible to have two threads adding/removing the same inode to the orphan list (otherwise the inode may not be on the orphan list at all since add/del is not refcounted).  The only reason for s_orphan_lock is to prevent corruption of the global list.
> 
> Yes, this is the best solution. Actually i've thought about that, but
> end up with more obvious fix for nojournal mode.
> With that we can avoid 1 of 3 locks on truncate in journal mode.
> I'll test that solution.

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