Re: [PATCH] ext4: optimize orphan_list handling for ext4_setattr

Andreas Dilger
2010-11-03T17:35:42Z

On 2010-11-03, at 03:31, Dmitry wrote:
> On Wed, 3 Nov 2010 03:06:41 -0600, Andreas Dilger <adilger@dilger.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