Re: [PATCH 0/3] ext4: don't use quota reservation for speculative metadata blocks

[Dmitry Monakhov <dmonakhov@openvz.org>]

Jan Kara <jack@suse.cz> writes:

>> > Eric Sandeen <sandeen@redhat.com> writes:
>> > 
>> > > Dmitry Monakhov wrote:
>> > >> Eric Sandeen <sandeen@redhat.com> writes:
>> > >> 
>> > >>> Because we can badly over-reserve metadata when we
>> > >>> calculate worst-case, it complicates things for quota, since
>> > >>> we must reserve and then claim later, retry on EDQUOT, etc.
>> > >>> Quota is also a generally smaller pool than fs free blocks,
>> > >>> so this over-reservation hurts more, and more often.
>> > >>>
>> > >>> I'm of the opinion that it's not the worst thing to allow
>> > >>> metadata to push a user slightly over quota.  This simplifies
>> > >>> the code and avoids the false quota rejections that result
>> > >>> from worst-case speculation.
>> > >> Hm.. Totally agree with issue description. And seem there is no another
>> > >> solution except yours.
>> > >> ASAIU alloc_nofail is called from places where it is impossible to fail
>> > >> an allocation even if something goes wrong.
>> > >> I ask because currently i'm working on EIO handling in alloc/free calls.
>> > >> I've found that it is useless to fail claim/free procedures because
>> > >> caller is unable to handle it properly.
>> > >> It is impossible to fail following operation
>> > >> ->writepage
>> > >>  ->dquot_claim_space (what to do if EIO happens?)
>> > >
>> > > Hm, if these start returning EIO then maybe my patch should be modified
>> > > to treat EDQUOT differently than EIO ... assuming callers can handle
>> > > the return at all.
>> > >
>> > > In other words, make NOFAIL really just mean "don't fail for EDQUOT"
>> > Yes. agree So we have two types of errors
>> > 1) expected errors: EDQUOT
>> > 2) fatal errors: (EIO/ENOSPC/ENOMEM)
>> > So we need two types of flags:
>> > 1)FORCE (IMHO it is better name than you proposed) to allow exceed a
>> >   quota limit
>> > 2)NOFAIL to allow ignore fatal errors.
>> > 
>> > We still need NOFAIL, because for example if something is happens in
>> > ->write_page()
>> >  ->dquot_claim()
>> >      update_quota() -> EIO  /* update disk quota */
>> >      update_bytes() /* update i_bytes count */
>> > It is obvious that write_page should fail because it is too late to
>> > return the error to userspace, so data will probably lost which
>> > is much more dramatic bug than quota inconsistency.
>> > So the only options we have is to:
>> > 1) Do not modify inode->i_bytes and return error which caller will
>> >    probably ignore. IMHO this is not good because result in
>> >    incorrect stat()
>> > 
>> > 2) do as much as we can (as it happens for now), modify inode->i_bytes
>> >    and return positive error code to caller.(which signal what error
>> >    result in quota inconsystency only)
>>   Yes, agreed that 2) is a better solution.
>> 
>> > This fatal errors handling logic i'll post on top of your patch-set.
>> > But please change flag name from NOFAIL to FORCE.
>>   Hmm, do we really need to distinguish between your NOFAIL and FORCE?
>> I mean there are places where we can handle quota failures (both EDQUOT
>> or others) and places where we cannot and then we just want to go on as
>> seamlessly as possible. So NOFAIL flag seems to be enough...
>>   Now I agree that in theory there can be some caller which might wish
>> to seamlessly continue on EDQUOT and bail out on EIO but I'm not aware
>> of such callsite currently so there's no immediate need for the flag.
>> So Eric's patches seem to be fine to me as they are. What do you think?
>   Maybe one more point - the callsite in ext4 that uses _nofail variant
> really needs full NOFAIL semantics because we have no way of handling
> an error there.
Yes. All places which Eric changed has (NOFAIL|FORCE) semantics.
But his patch spotted other places where we can use only FORCE.