[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex
From: |
Stefan Hajnoczi |
Subject: |
Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex |
Date: |
Thu, 8 Jul 2021 14:04:21 +0100 |
On Thu, Jul 08, 2021 at 01:32:12PM +0200, Paolo Bonzini wrote:
> On 08/07/21 12:36, Stefan Hajnoczi wrote:
> > > What is very clear from this patch is that it
> > > is strictly related to the brdv_* and lower level calls, because
> > > they also internally check or even use the aiocontext lock.
> > > Therefore, in order to make it work, I temporarly added some
> > > aiocontext_acquire/release pair around the function that
> > > still assert for them or assume they are hold and temporarly
> > > unlock (unlock() - lock()).
> >
> > Sounds like the issue is that this patch series assumes AioContext locks
> > are no longer required for calling the blk_*()/bdrv_*() APIs? That is
> > not the case yet, so you had to then add those aio_context_lock() calls
> > back in elsewhere. This approach introduces unnecessary risk. I think we
> > should wait until blk_*()/bdrv_*() no longer requires the caller to hold
> > the AioContext lock before applying this series.
>
> In general I'm in favor of pushing the lock further down into smaller and
> smaller critical sections; it's a good approach to make further audits
> easier until it's "obvious" that the lock is unnecessary. I haven't yet
> reviewed Emanuele's patches to see if this is what he's doing where he's
> adding the acquire/release calls, but that's my understanding of both his
> cover letter and your reply.
The problem is the unnecessary risk. We know what the goal is for
blk_*()/bdrv_*() but it's not quite there yet. Does making changes in
block jobs help solve the final issues with blk_*()/bdrv_*()?
If yes, then it's a risk worth taking. If no, then spending time
developing interim code, reviewing those patches, and risking breakage
doesn't seem worth it. I'd rather wait for blk_*()/bdrv_*() to be fully
complete and then see patches that delete aio_context_acquire() in most
places or add locks in the remaining places where the caller was relying
on the AioContext lock.
Stefan
signature.asc
Description: PGP signature
- [RFC PATCH 4/6] job.h: categorize job fields, (continued)
- [RFC PATCH 4/6] job.h: categorize job fields, Emanuele Giuseppe Esposito, 2021/07/07
- [RFC PATCH 5/6] job: use global job_mutex to protect struct Job, Emanuele Giuseppe Esposito, 2021/07/07
- [RFC PATCH 6/6] jobs: remove unnecessary AioContext aquire/release pairs, Emanuele Giuseppe Esposito, 2021/07/07
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Stefan Hajnoczi, 2021/07/08
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Paolo Bonzini, 2021/07/08
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Kevin Wolf, 2021/07/08
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex,
Stefan Hajnoczi <=
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Emanuele Giuseppe Esposito, 2021/07/12
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Stefan Hajnoczi, 2021/07/13
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Vladimir Sementsov-Ogievskiy, 2021/07/13
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Stefan Hajnoczi, 2021/07/13
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Vladimir Sementsov-Ogievskiy, 2021/07/15
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Stefan Hajnoczi, 2021/07/15
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Kevin Wolf, 2021/07/16
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Stefan Hajnoczi, 2021/07/19
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Kevin Wolf, 2021/07/19
Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Stefan Hajnoczi, 2021/07/08