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

[Dmitry <dmonakhov@openvz.org>]

On Wed, 3 Nov 2010 03:06:41 -0600, Andreas Dilger <adilger@dilger.ca> wrote:
> On 2010-10-25, at 01:06, Dmitry Monakhov wrote:
> > Surprisingly chown() on ext4 is not SMP scalable operation. 
> > Due to unconditional orphan_del(NULL, inode) in ext4_setattr()
> > result in significant performance overhead because of global orphan
> > mutex, especially in no-journal mode (where orphan_add() is noop).
> > It is possible to skip explicit orphan_del if possible.
> > 
> > Results of fchown() micro-benchmark in no-journal mode
> > while (1) {
> >   iteration++;
> >   fchown(fd, uid, gid);
> >   fchown(fd, uid + 1, gid + 1)
> > }
> > measured: iterations per millisecond
> > | nr_tasks | w/o patch | with patch |
> > |        1 |       142 |        185 |
> > |        4 |       109 |        642 |
> 
> AFAICS, both ext4_orphan_add() and ext4_orphan_del() already check right at the top of the function whether the handle is valid, so I can't really understand how this patch would make any difference for no-journal mode.
> 
> > @@ -5636,7 +5640,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr 
> 
> > +		if (ext4_handle_valid(handle)) {
> > +			error = ext4_orphan_add(handle, inode);
> > +			orphan = 1;
> > +		}
> > 		EXT4_I(inode)->i_disksize = attr->ia_size;
> > 		rc = ext4_mark_inode_dirty(handle, inode);
> > 		if (!error)
> 
> > @@ -5636,7 +5640,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr
> > 	 * If the call to ext4_truncate failed to get a transaction handle at
> > 	 * all, we need to clean up the in-core orphan list manually.
> > 	 */
> > -	if (inode->i_nlink)
> > +	if (orphan && inode->i_nlink)
> > 		ext4_orphan_del(NULL, inode);
> > 
> > 	if (!rc && (ia_valid & ATTR_MODE))
> 
> 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.
> 
> I think the actual fix would be to always set "orphan = 1" after
> ext4_orphan_add() is called,
But in case of nojournal mode it will be called with noop result.
but orphan_del(NULL, inode) will result in useless locking. 
> and only call ext4_orphan_del() in that case.  This is essentially what your patch does for with-journal mode, and the non-journal mode is a red-herring due to the early exit from ext4_orphan_del().
> 
> 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
endup 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html