[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] block/file-posix: File locking
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] block/file-posix: File locking during creation |
Date: |
Sat, 28 Apr 2018 13:03:54 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
On 2018-04-27 08:22, Fam Zheng wrote:
> On Sat, 04/21 00:09, Max Reitz wrote:
>> When creating a file, we should take the WRITE and RESIZE permissions.
>> We do not need either for the creation itself, but we do need them for
>> clearing and resizing it. So we can take the proper permissions by
>> replacing O_TRUNC with an explicit truncation to 0, and by taking the
>> appropriate file locks between those two steps.
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>> block/file-posix.c | 41 +++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index c98a4a1556..ed7932d6e8 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1992,6 +1992,7 @@ static int raw_co_create(BlockdevCreateOptions
>> *options, Error **errp)
>> {
>> BlockdevCreateOptionsFile *file_opts;
>> int fd;
>> + int perm, shared;
>> int result = 0;
>>
>> /* Validate options and set default values */
>> @@ -2006,14 +2007,48 @@ static int raw_co_create(BlockdevCreateOptions
>> *options, Error **errp)
>> }
>>
>> /* Create file */
>> - fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_TRUNC |
>> O_BINARY,
>> - 0644);
>> + fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, 0644);
>> if (fd < 0) {
>> result = -errno;
>> error_setg_errno(errp, -result, "Could not create file");
>> goto out;
>> }
>>
>> + /* Take permissions: We want to discard everything, so we need
>> + * BLK_PERM_WRITE; and truncation to the desired size requires
>> + * BLK_PERM_RESIZE.
>> + * On the other hand, we cannot share the RESIZE permission
>> + * because we promise that after this function, the file has the
>> + * size given in the options. If someone else were to resize it
>> + * concurrently, we could not guarantee that. */
>
> Strictly speaking, we do close(fd) before this function returns so the lock is
> lost and race can happen.
Right, but then creation from the perspective of file-posix is over. We
are going to reopen the file for formatting, but that is just a normal
bdrv_open() so it will automatically be locked, no?
>> + perm = BLK_PERM_WRITE | BLK_PERM_RESIZE;
>> + shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
>> +
>> + /* Step one: Take locks in shared mode */
>> + result = raw_apply_lock_bytes(fd, perm, shared, false, errp);
>> + if (result < 0) {
>> + goto out_close;
>> + }
>> +
>> + /* Step two: Try to get them exclusively */
>> + result = raw_check_lock_bytes(fd, perm, shared, errp);
>> + if (result < 0) {
>> + goto out_close;
>> + }
>> +
>> + /* Step three: Downgrade them to shared again, and keep
>> + * them that way until we are done */
>> + result = raw_apply_lock_bytes(fd, perm, shared, true, errp);
>
> You comment it as "downgrade to shared" but what this actually does in
> addition
> to step one is to "unlock" unneeded bytes which we haven't locked anyway. So
> I'm
> confused why we need this stop. IIUC no downgrading is necessary after step
> one
> and step two: only necessary shared locks are acquired in step one, and step
> two
> is read-only op (F_OFD_GETLK).
Oops. For some reason I thought raw_check_lock_bytes() would take the
locks exclusively. Yes, then we don't need this third step.
Thanks for reviewing!
Max
>> + if (result < 0) {
>> + goto out_close;
>> + }
>> +
>> + /* Clear the file by truncating it to 0 */
>> + result = raw_regular_truncate(fd, 0, PREALLOC_MODE_OFF, errp);
>> + if (result < 0) {
>> + goto out_close;
>> + }
>> +
>> if (file_opts->nocow) {
>> #ifdef __linux__
>> /* Set NOCOW flag to solve performance issue on fs like btrfs.
>> @@ -2029,6 +2064,8 @@ static int raw_co_create(BlockdevCreateOptions
>> *options, Error **errp)
>> #endif
>> }
>>
>> + /* Resize and potentially preallocate the file to the desired
>> + * final size */
>> result = raw_regular_truncate(fd, file_opts->size,
>> file_opts->preallocation,
>> errp);
>> if (result < 0) {
>> --
>> 2.14.3
>>
>>
>
> Fam
>
signature.asc
Description: OpenPGP digital signature