On Sat, 04/28 13:03, Max Reitz wrote:
> 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 <mre...@redhat.com>
> >> ---
> >> 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?
After this function close() but before the following bdrv_open(), another
process can sneak in and steal the lock. So we cannot guarantee the RESIZE lock
does what we really want.
Fam
>
> >> + 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
> >
>
>
