[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] backup: don't acquire aio_context in backup_clean

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH] backup: don't acquire aio_context in backup_clean
Date: Thu, 26 Mar 2020 15:46:18 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

26.03.2020 12:43, Stefan Reiter wrote:
On 26/03/2020 06:54, Vladimir Sementsov-Ogievskiy wrote:
25.03.2020 18:50, Stefan Reiter wrote:
backup_clean is only ever called as a handler via job_exit, which

Hmm.. I'm afraid it's not quite correct.



      job_completed_txn_abort (lock aio context)


Hmm. job_do_finalize calls job_completed_txn_abort, which cares to lock aio 
And on the same time, it directaly calls job_txn_apply(job->txn, 
without locking. Is it a bug?

I think, as you say, the idea is that job_do_finalize is always called with the lock 
acquired. That's why job_completed_txn_abort takes care to release the lock (at least of 
the "outer_ctx" as it calls it) before reacquiring it.

And, even if job_do_finalize called always with locked context, where is 
guarantee that all
context of all jobs in txn are locked?

I also don't see anything that guarantees that... I guess it could be adapted 
to handle locks like job_completed_txn_abort does?

Haven't looked into transactions too much, but does it even make sense to have 
jobs in different contexts in one transaction?

Why not? Assume backing two disks in one transaction, each in separate io 
thread.. (honestly, I don't know does it work)

Still, let's look through its callers.


                qmp_block_job_finalize (lock aio context)
                qmp_job_finalize (lock aio context)
                test_cancel_concluded (doesn't lock, but it's a test)



                     job_exit (lock aio context)


                          blockdev_mark_auto_del (lock aio context)


                              qmp_block_job_cancel (locks context)
                              qmp_job_cancel  (locks context)


                               job_cancel_sync (return job_finish_sync(job, 
&job_cancel_err, NULL);, job_finish_sync just calls callback)

                                    replication_close (it's .bdrv_close.. Hmm, 
I don't see context locking, where is it ?)
Hm, don't see it either. This might indeed be a way to get to job_clean without 
a lock held.

I don't have any testing set up for replication atm, but if you believe this 
would be correct I can send a patch for that as well (just acquire the lock in 
replication_close before job_cancel_async?).

I don't know.. But sending a patch is good way to start a discussion)

                                    replication_stop (locks context)

                                    drive_backup_abort (locks context)

                                    blockdev_backup_abort (locks context)

                                    job_cancel_sync_all (locks context)

                                    cancel_common (locks context)

                          test_* (I don't care)

To clarify, aside from the commit message the patch itself does not appear to 
be wrong? All paths (aside from replication_close mentioned above) guarantee 
the job lock to be held.

I mostly worry about the case with transaction with jobs from different aio 
contexts than about replication..

Anyway, I hope that someone who has better understanding of these things will 
look at this.

It usually not good idea to send [PATCH] inside discussion thread, it'd better 
be a separate thread, to be more visible.

May be you send separate series, which will include this patch, some fix for 
replication, and try to fix job_do_finalize in some way, and we continue 
discussion from this new series?

already acquires the job's context. The job's context is guaranteed to
be the same as the one used by backup_top via backup_job_create.

Since the previous logic effectively acquired the lock twice, this
broke cleanup of backups for disks using IO threads, since the BDRV_POLL_WHILE
in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock
once, thus deadlocking with the IO thread.

Signed-off-by: Stefan Reiter <address@hidden>

Just note, that this thing were recently touched by 0abf2581717a19 , so add 
Sergio (its author) to CC.


This is a fix for the issue discussed in this part of the thread:
...not the original problem (core dump) posted by Dietmar.

I've still seen it occasionally hang during a backup abort. I'm trying to figure
out why that happens, stack trace indicates a similar problem with the main
thread hanging at bdrv_do_drained_begin, though I have no clue why as of yet.

  block/backup.c | 4 ----
  1 file changed, 4 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 7430ca5883..a7a7dcaf4c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -126,11 +126,7 @@ static void backup_abort(Job *job)
  static void backup_clean(Job *job)
      BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-    AioContext *aio_context = bdrv_get_aio_context(s->backup_top);
-    aio_context_acquire(aio_context);
-    aio_context_release(aio_context);
  void backup_do_checkpoint(BlockJob *job, Error **errp)

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]