[Top][All Lists]

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

Re: [PATCH v4 00/25] block layer: split block APIs in global state and I

From: Emanuele Giuseppe Esposito
Subject: Re: [PATCH v4 00/25] block layer: split block APIs in global state and I/O
Date: Fri, 19 Nov 2021 11:42:52 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

On 19/11/2021 04:13, Paolo Bonzini wrote:

El jue., 18 nov. 2021 16:31, Hanna Reitz <hreitz@redhat.com <mailto:hreitz@redhat.com>> escribió:

    On 18.11.21 14:50, Paolo Bonzini wrote:
     > On 11/15/21 17:03, Hanna Reitz wrote:
     >> I only really see four solutions for this:
     >> (1) We somehow make the amend job run in the main context under the
     >> BQL and have it prevent all concurrent I/O access (seems bad)
     >> (2) We can make the permission functions part of the I/O path
     >> wrong and probably impossible?)
     >> (3) We can drop the permissions update and permanently require the
     >> permissions that we need when updating keys (I think this might
     >> existing use cases)
     >> (4) We can acquire the BQL around the permission update call and
     >> perhaps that works?
     >> I don’t know how (4) would work but it’s basically the only
     >> reasonable solution I can come up with.  Would this be a way to
     >> a BQL function from an I/O function?
     > I think that would deadlock:
     >     main                I/O thread
     >     --------            -----
     >     start bdrv_co_amend
     >                     take BQL
     >     bdrv_drain
     >     ... hangs ...


    Is there really nothing we can do?  Forgive me if I’m talking complete
    nonsense here (because frankly I don’t even really know what a bottom
    half is exactly), but can’t we schedule some coroutine in the main
    thread to do the perm notifications and wait for them in the I/O thread?

I think you still get a deadlock, just one with a longer chain. You still have a cycle of things depending on each other, but one of them is now the I/O thread waiting for the bottom half.

    Hmm...  Perhaps.  We would need to undo the permission change when the
    job finishes, though, i.e. in JobDriver.prepare() or JobDriver.clean().
    Doing the change in qmp_x_blockdev_amend() would be asymmetric then, so
    we’d probably want a new JobDriver method that runs in the main thread
    before .run() is invoked. (Unfortunately, “.prepare()” is now taken

Ok at least it's feasible.

Ok I think I got it. I will create a new callback, maybe "pre_run" or something like that to perform the first bdrv_child_refresh_perms and implement the .clean callback to perform the "cleanup" bdrv_child_refresh_perms in block_crypto_amend_options_generic_luks.

    Doesn’t solve the FUSE problem, but there we could try to just take the
    RESIZE permission permanently and if that fails, we just don’t allow
    truncates for that export.  Not nice, but should work for common cases.

Yeah definitely not nice. Probably permissions could be protected by their own mutex, even a global one like the one we have for jobs. For now I suggest just ignoring the problem and adding a comment, since it's not really something that didn't exist.

Will add a TODO in blk_set/get permissions explaining the issue.
Last issue we had with regards to permissions in GS had to do with bdrv_co_invalidate_cache: however, Paolo suggested me a simple fix to simply assert that the function is either under BQL or does not have open_flags & BDRV_O_INACTIVE set. This basically skips the permission code block, entering it only if we have the BQL.

Ok, apart from this permissions issue and assert_bdrv_graph_writable, I should have addressed all main comments of this series. Assume that for the others where I did not explicitly answered, I agree and applied your comments.

Thank you,

reply via email to

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