> On May 25, 2023, at 5:26 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> On Thu, May 25, 2023 at 05:13:41PM +0000, Wengang Wang wrote:
>>
>>
>>> On May 24, 2023, at 7:20 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>>>
>>> On Wed, May 24, 2023 at 04:27:19PM +0000, Wengang Wang wrote:
>>>>> On May 23, 2023, at 5:27 PM, Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>>>>> On Tue, May 23, 2023 at 02:59:40AM +0000, Wengang Wang wrote:
>>>>>>> On May 22, 2023, at 3:40 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>>>>>>> On Mon, May 22, 2023 at 06:20:11PM +0000, Wengang Wang wrote:
>>>>>>>>> On May 21, 2023, at 6:17 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>>>>>>>>> On Fri, May 19, 2023 at 10:18:29AM -0700, Wengang Wang wrote:
>>>>>>>>>> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
>>>>>>>> For existing other cases (if there are) where new intents are added,
>>>>>>>> they don’t use the capture_list for delayed operations? Do you have example then?
>>>>>>>> if so I think we should follow their way instead of adding the defer operations
>>>>>>>> (but reply on the intents on AIL).
>>>>>>>
>>>>>>> All of the intent recovery stuff uses
>>>>>>> xfs_defer_ops_capture_and_commit() to commit the intent being
>>>>>>> replayed and cause all further new intent processing in that chain
>>>>>>> to be defered until after all the intents recovered from the journal
>>>>>>> have been iterated. All those new intents end up in the AIL at a LSN
>>>>>>> index >= last_lsn.
>>>>>>
>>>>>> Yes. So we break the AIL iteration on seeing an intent with lsn equal to
>>>>>> or bigger than last_lsn and skip the iop_recover() for that item?
>>>>>> and shall we put this change to another separated patch as it is to fix
>>>>>> an existing problem (not introduced by my patch)?
>>>>>
>>>>> Intent replay creates non-intent log items (like buffers or inodes) that
>>>>> are added to the AIL with an LSN higher than last_lsn. I suppose it
>>>>> would be possible to break log recovery if an intent's iop_recover
>>>>> method immediately logged a new intent and returned EAGAIN to roll the
>>>>> transaction, but none of them do that;
>>>>
>>>> I am not quite sure for above. There are cases that new intents are added
>>>> in iop_recover(), for example xfs_attri_item_recover():
>>>>
>>>> 632 error = xfs_xattri_finish_update(attr, done_item);
>>>> 633 if (error == -EAGAIN) {
>>>> 634 /*
>>>> 635 * There's more work to do, so add the intent item to this
>>>> 636 * transaction so that we can continue it later.
>>>> 637 */
>>>> 638 xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_ATTR, &attr->xattri_list);
>>>> 639 error = xfs_defer_ops_capture_and_commit(tp, capture_list);
>>>> 640 if (error)
>>>> 641 goto out_unlock;
>>>> 642
>>>> 643 xfs_iunlock(ip, XFS_ILOCK_EXCL);
>>>> 644 xfs_irele(ip);
>>>> 645 return 0;
>>>> 646 }
>>>>
>>>> I am thinking line 638 and 639 are doing so.
>>>
>>> I don't think so. @attrip is the attribute information recovered
>>> from the original intent - we allocated @attr in
>>> xfs_attri_item_recover() to enable the operation indicated by
>>> @attrip to be executed. We copy stuff from @attrip (the item we are
>>> recovering) to the new @attr structure (the work we need to do) and
>>> run it through the attr modification state machine. If there's more
>>> work to be done, we then add @attr to the defer list.
>>
>> Assuming “if there’s more work to be done” is the -EAGAIN case...
>>
>>>
>>> If there is deferred work to be continued, we still need a new
>>> intent to indicate what attr operation needs to be performed next in
>>> the chain. When we commit the transaction (639), it will create a
>>> new intent log item for the deferred attribute operation in the
>>> ->create_intent callback before the transaction is committed.
>>
>> Yes, that’s true. But it has no conflict with what I was saying. The thing I wanted
>> to say is that:
>> The new intent log item (introduced by xfs_attri_item_recover()) could appear on
>> AIL before the AIL iteration ends in xlog_recover_process_intents().
>> Do you agree with above?
>
> Recovery of intents has *always* been able to do this. What I don't
> understand is why you think this is a problem.
Adding new intent is not a problem.
The problem I am thinking is that for the NEW intents,
we should’d run both:
1. iop_recover() during the AIL intents iteration and
2. the ops->finish_item() in xlog_finish_defer_ops()’s sub calls()
I am thinking the iop_recover() and the finish_item() are doing same thing.
I am not quite sure about ATTRI but I am sure about EFI. For EFI,
both xfs_efi_item_recover() (as iop_recover()) and xfs_extent_free_finish_item()
(as finish_item()) calls xfs_trans_free_extent() to free the same extent(s).
I have a metadump with which I can reproduce the original AGFL deadlock problem
during log recovery. When I tested my patch (without the changes in
xlog_recover_process_intents()), I found the records in the NEW EFI are freed twice,
one is during the AIL intents iteration with iop_recover(), the other is inside
xlog_finish_defer_ops() with finish_item().
That needs to be fixed. The best way to fix that I thought is to fix xlog_recover_process_intents().
As log recovery, I am thinking the iop_cover() should only applied the original on disk redo intents
only. Those original redo intents has no deferred option attached. And for the NEW intents
introduced during the processing of the original intents, I don’t think iop_recover() should
run for them becuase
1. they are NEW ones, not the original on disk redo intents and
2. they have deferred operation attached which process the new intents later.
And I am also thinking we also depend xlog_recover_process_intents() to run iop_recover()
on the new intents. That’s because the new intents could be already inserted to AIL
before the AIL intents iteration ends, in that case have the change to run iop_recover() on the
new ones. but the new intents also could be inserted to AIL after the AIL intents iteration ends,
in that case we have no change to run iop_recover() on the new intents. — that’s an unstable thing.
>
>> In the following I will talk about the new intent log item.
>>
>> The iop_recover() is then executed on the new intent during the AIL intents iteration.
>> I meant this loop:
>>
>> 2548 for (lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
>> 2549 lip != NULL;
>> 2550 lip = xfs_trans_ail_cursor_next(ailp, &cur)) {
>> ….
>> 2584 }
>> Do you agree with above?
>
> Repeatedly asserting that I must agree that your logic is correct
> comes across as unecessarily aggressive and combative and is not
> acceptible behaviour.
>
> Aggressively asserting that you are right doesn't make your logic
> any more compelling. It just makes you look really bad when,
> inevitably, you get politely told that you've made yet another
> mistake.
Nope, I didn’t want to be aggressive. Since this is already discussed a lot
but we still don’t have an agreement. So I wanted to know where is the disagreement,
little by little. I thought it would be a good way to make things clear,
if it made you unhappy, sorry, and I didn’t mean that.
>
>> And the lsn for the new intent is equal to or bigger than last_lsn.
>> Do you agree with above?
>>
>> In above case the iop_recover() is xfs_attri_item_recover(). The later
>> creates a xfs_attr_intent, attr1, copying things from the new ATTRI to attr1
>> and process attr1.
>> Do you agree with above?
>>
>> Above xfs_attri_item_recover() runs successfully and the AIL intent iteration ends.
>
> We ran xfs_xattri_finish_update(attr, done_item) which returned
> -EAGAIN. This means we ran xfs_xattri_finish_update() and modified
> -something- before we logged the new intent. That, however, doesn't
> dictate what we need to log in the new intent - that is determined
> by the overall intent chain and recovery architecture the subsystem
> uses. And the attribute modification architecture is extremely
> subtle, complex and full of unexpected loops.
I am a bit confused here.
I know that xfs_efi_item_recover() and xfs_extent_free_finish_item() are doing same thing
(free the extent specified in the EFI record(s)). So whatever the new would be logged int
the new intents, I am thinking the for the same new ATTRI intent, xfs_attri_item_recover()
and xfs_attr_finish_item() should do the same thing. If they don’t not, ATTIR has inconsistent
behavior with EFI.
>
>> Now it’s time to process the capture_list with xlog_finish_defer_ops(). The
>> capture_list contains the deferred operation which was added at line 639 with type
>> XFS_DEFER_OPS_TYPE_ATTR.
>> Do you agree with above?
>>
>> In xlog_finish_defer_ops(), a new transaction is created by xfs_trans_alloc().
>> the deferred XFS_DEFER_OPS_TYPE_ATTR operation is attached to that new
>> transaction by xfs_defer_ops_continue(). Then the new transaction is committed by
>> xfs_trans_commit().
>> Do you agree with above?
>
> So at this point it we are logging the same intent *information*
> again. But it's not the same intent - we've retired the original
> intent and we have a new intent chain for the operations we are
> going to perform. But why are we logging the same information in a
> new intent?
>
> Well, the attr modification state machine was -carefully- designed
> to work this way: if we crash part way through an intent chain, we
> always need to restart it in recovery with a "garbage collection"
> step that undoes the partially complete changes that had been made
> before we crashed. Only once we've done that garbage collection step
> can we restart the original operation the intent described.
>
> Indeed, the operations we perform recovering an attr intent are
> actually different to the operations we perform with those intents
> at runtime. Recovery has to perform garbage collection as a first
> step, yet runtime does not.
>
> IOWs, xfs_attri_item_recover() -> xfs_xattri_finish_update() is
> typically performing garbage collection in the first recovery
> transaction. If it does perform a gc operation, then it is
> guaranteed to return -EAGAIN so that the actual operation recovery
> needs to perform can have new intents logged, be deferred and then
> eventually replayed.
>
> If we don't need to perform gc, then the operation we perform may
> still require multiple transactions to complete and we get -EAGAIN
> in that case, too. When that happens, we need to ensure if we crash
> before recovery completes that the next mount will perform the
> correct GC steps before it replays the intent from scratch. So we
> always need to log an intent for the xattr operation that will
> trigger the GC step on recovery.
>
> Either way, we need to log new intents that are identical to the
> original one at each step of the process so that log recovery will
> always start the intent recovery process with the correct garbage
> collection operation....
>
>> During the xfs_trans_commit(), xfs_defer_finish_noroll() is called. and
>> xfs_defer_finish_one() is called inside xfs_defer_finish_noroll(). For the
>> deferred XFS_DEFER_OPS_TYPE_ATTR operation, xfs_attr_finish_item()
>> is called.
>> Do you agree with above?
>>
>> In xfs_attr_finish_item(), xfs_attr_intent (attr2) is taken out from above new ATTRI
>> and is processed. attr2 and attr1 contain exactly same thing because they are both
>> from the new ATTRI. So the processing on attr2 is pure a duplication of the processing
>> on attr1. So actually the new ATTRI are double-processed.
>> Do you agree with above?
>
> No, that's wrong. There is now "attr1 and attr2" structures - they
> are one and the same. i.e. the xfs_attr_intent used in
> xfs_attr_finish_item() is the same one created in
> xfs_attri_item_recover() that we called
> xfs_defer_add(&attr->xattri_list) with way back at line 638.
>
> xfs_attr_finish_item(
> struct xfs_trans *tp,
> struct xfs_log_item *done,
> struct list_head *item,
> struct xfs_btree_cur **state)
> {
> struct xfs_attr_intent *attr;
> struct xfs_attrd_log_item *done_item = NULL;
> int error;
>
>>>>>> attr = container_of(item, struct xfs_attr_intent, xattri_list);
> if (done)
> done_item = ATTRD_ITEM(done);
>
> xfs_attr_finish_item() extracts the @attr structure from the
> listhead that is linking it into the deferred op chain, and we
> run xfs_attr_set_iter() again to run the next step(s) in the xattr
> modification state machine. If this returns -EAGAIN (and it can)
> we go around the "defer(attr->attri_list), create new intent,
> commit, ... ->finish_item() -> xfs_attr_finish_item()" loop again.
>
> From this, I am guessing that you are confusing the per-defer-cycle
> xfs_attri_log_item with the overall xfs_attr_intent context. i.e. We
> allocate a new xfs_attri_log_item (and xfs_attrd_log_item) every
> time we create new intents in the deferred operation chain. Each
> xfs_attri_log_item is created from the state that is carried
> across entire the attr recovery chain in the xfs_attr_intent
> structure....
OK, ATTRI really looks complex. And it looks like the
xfs_attri_item_recover() and xfs_attr_finish_item() can do different things
according the current state. Thus running both xfs_attri_item_recover()
and xfs_attr_finish_item() has no problem. I didn’t take a good example :(.
But, the original
ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);
in xlog_recover_process_intents() is incorrect, right? For example if we
introduced two different new ATTRIs and the 2nd should have li_lsn bigger
than last_lsn?
Anyway, let’s go back to the EFI. EFI is different from ATTRI, it has no state machine.
xfs_efi_item_recover() and xfs_extent_free_finish_item() are doing the same thing —
free the extent specified by the EFI record(s). For the new EFI, the
xfs_efi_item_recover() goes well, but xfs_extent_free_finish_item() fails because it
want to free the extent which is already freed by the xfs_efi_item_recover().
We pass some information (indicating it already done) from xfs_efi_item_recover() to
xfs_extent_free_finish_item() somehow so that xfs_extent_free_finish_item() won’t do
the 2nd free as a workaround? What’s your idea?
thanks,
wengang
