Dmitry Monakhov <dmonakhov@xxxxxxxxxx> writes:
> Dmitry Monakhov <dmonakhov@xxxxxxxxxx> writes:
>
>> Jan Kara <jack@xxxxxxx> writes:
>>
>>> On Fri 26-03-10 11:18:28, Dmitry Monakhov wrote:
>>>> > If there isn't a reason to continue using unjournaled quota (i.e. it
>>>> > doesn't break to just move to journaled quota everywhere), then these
>>>> > could just become aliases for the journaled quota implementation. The
>>>> > other alternative is to deprecate these options in the next kernel and
>>>> > have it print out a warning on the console to tell the user to switch
>>>> > over to the journaled version.
>>>> The only reason to not use journalled quota by default is the currently
>>>> it is a bit slower than unjournalled variant.
>>>> This is because each quota change result in synchronous quotafile
>>>> update in per-sb-page-cache. And this update is protected by i_mutex.
>>>> and dqio_mutex. It may be fixed easily. I've sent a RFC patch two
>>>> month ago. I'll update it and will submit it this weekend.
>>> Well, there is also some overhead caused by more IO we have to do for
>>> quota journaling and that is essentially unavoidable. But still I believe
>>> we should transition people to journaled quotas...
>> Agree. IO overhead due to journalled quota is almost invisible.
>> And it must be enabled by default after most annoying lock contention
>> will be resolved.
>>
>> BTW. i've had bad news. Seems what journalled was broken recently.
>> Right after i've wrote the first letter. i've started to update the
>> quota-speedup patch. And during testing phase i've found that
>> journalled quota is inconsistent after power-failure(w/o my patches).
>> I've tested ext4.git/for-next branch
>> Currently i'm investing the issue.
> Ok, i've found the root of issue. dquot_transfer() wasn't called for
> symlinks on chown due to lack of ->setattr operation.
> Before 'dquot: cleanup dquot transfer routine' patch quota_transfer()
> was performed by notify_transfer() itself.
Forgot to mention that it is not journalled quota issue. But just a
generic quota regression.
> Now it must be handled by in corresponding ->setattr
>
> BTW i'm wondering, even if we don't care about quota. Inode's attributes
> are metadata and must goes trough journal(i.e via extXXX_setattr).
> so every inode type must has corresponding ->setattr.
As is is always happens. Each modification result in unexpected regressions.
In case of quota cleanup patch-set movement of quota-transfer from
generic-setattr to fs-speciffic ->setattr result in hidden regression
because not all inode types has correct ->setattr methods.
Where are too many filesystems to look-at. Let's add a some
sanity check in to notify_changes(), and remove it after 2/3 moths.
Some thing like this:
static int quota_check(struct inode *inode, struct iattr *attr)
{
if (!sb_any_quota_active(inode->i_sb))
return 0;
if (((attr->ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
(attr->ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid) ||
(attr->ia_valid & ATTR_SIZE)) && !inode->i_op->setattr)
{
WARN_ON(1);
return 1;
}
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
