From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Georgy Kirichenko <georgy@tarantool.org>,
tarantool-patches@dev.tarantool.org, korablev@tarantool.org,
imun@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically
Date: Mon, 2 Mar 2020 21:51:10 +0100 [thread overview]
Message-ID: <66f6cdbb-03b8-14ac-5b53-58d9919740ee@tarantool.org> (raw)
In-Reply-To: <4600369.31r3eYUQgx@localhost>
It was decided, that Georgy is right. I will create a new
commit on this branch, which introduces a special fiber for
delayed GC. I will use it in SWIM and in fio.
So please, skip this version. A new one will be send as v2.
On 09/02/2020 19:34, Georgy Kirichenko wrote:
> Hi, thanks for the patch.
>
> Unfortunately your patch is not correct because fiber.create
> yields (which could be fixed by using fiber.new) and raises an error what is not
> allowed inside gc callbacks.
>
> From my point of view the possible approach is a dedicated fiber with
> a list of pending callbacks.
>
> WBR
>
> On Sunday, 9 February 2020 20:37:09 MSK Vladislav Shpilevoy wrote:
>> Fio.open() returned a file descriptor, which was not closed
>> automatically after all its links were nullified. In other words,
>> GC didn't close the descriptor.
>>
>> This was not really useful, because after fio.open() an exception
>> may appear, and user needed to workaround this to manually call
>> fio_object:close(). Also this was not consistent with io.open().
>>
>> Now fio.open() object closes the descriptor automatically when
>> GCed.
>>
>> Closes #4727
>>
>> @TarantoolBot document
>> Title: fio descriptor is closed automatically by GC
>>
>> fio.open() returns a descriptor which can be closed manually by
>> calling :close() method, or it will be closed automatically, when
>> it has no references, and GC deletes it.
>>
>> :close() method existed always, auto GC was added just now.
>>
>> ---
>>
>> @ChangeLog
>> - fio descriptors are closed on garbage collection (gh-4727).
>>
>> src/lua/fio.lua | 33 ++++++++++++---
>> test/app/fio.result | 93 +++++++++++++++++++++++++++++++++++++++++++
>> test/app/fio.test.lua | 58 +++++++++++++++++++++++++++
>> 3 files changed, 178 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
>> index 4692e1026..41901904c 100644
>> --- a/src/lua/fio.lua
>> +++ b/src/lua/fio.lua
>> @@ -141,10 +141,12 @@ end
>>
>> fio_methods.close = function(self)
>> local res, err = internal.close(self.fh)
>> - self.fh = -1
>> if err ~= nil then
>> return false, err
>> end
>> + ffi.gc(self._gc, nil)
>> + self._gc = nil
>> + self.fh = -1
>> return res
>> end
>>
>> @@ -160,7 +162,23 @@ fio_methods.stat = function(self)
>> return internal.fstat(self.fh)
>> end
>>
>> -local fio_mt = { __index = fio_methods }
>> +local fio_mt = {
>> + __index = fio_methods,
>> + __serialize = function(obj)
>> + return {fh = obj.fh}
>> + end,
>> +}
>> +
>> +local function fio_wrap(fh)
>> + return setmetatable({
>> + fh = fh,
>> + _gc = ffi.gc(ffi.new('char[1]'), function()
>> + -- FFI GC can't yield. Internal.close() yields.
>> + -- Collect the garbage later, in a separate fiber.
>> + fiber.create(internal.close, fh)
>> + end)
>> + }, fio_mt)
>> +end
>>
>> fio.open = function(path, flags, mode)
>> local iflag = 0
>> @@ -202,10 +220,13 @@ fio.open = function(path, flags, mode)
>> if err ~= nil then
>> return nil, err
>> end
>> -
>> - fh = { fh = fh }
>> - setmetatable(fh, fio_mt)
>> - return fh
>> + local ok, res = pcall(fio_wrap, fh)
>> + if not ok then
>> + internal.close(fh)
>> + -- This is either OOM or bad syntax, both require throw.
>> + return error(res)
>> + end
>> + return res
>> end
>>
>> fio.pathjoin = function(...)
>> diff --git a/test/app/fio.result b/test/app/fio.result
>> index f83c43f44..6345ac22e 100644
>> --- a/test/app/fio.result
>> +++ b/test/app/fio.result
>> @@ -1456,3 +1456,96 @@ fio.mktree('/dev/null/dir')
>> - false
>> - 'Error creating directory /dev/null: File exists'
>> ...
>> +--
>> +-- gh-4727: fio handler GC.
>> +--
>> +flags = {'O_CREAT', 'O_RDWR'}
>> +---
>> +...
>> +mode = {'S_IRWXU'}
>> +---
>> +...
>> +filename = 'test4727.txt'
>> +---
>> +...
>> +fh1 = nil
>> +---
>> +...
>> +fh2 = nil
>> +---
>> +...
>> +-- Idea of the test is that according to the Open Group standard,
>> +-- open() always returns the smallest available descriptor. This
>> +-- means, that in 'open() + close() + open()' the second open()
>> +-- should return the same value as the first call, if no other
>> +-- threads/fibers managed to interfere. Because of the
>> +-- interference the sequence may need to be called multiple times
>> +-- to catch a couple of equal descriptors.
>> +-- GC function of a fio object creates a new fiber. Give it \
>> +-- time to execute. \
>> +test_run:wait_cond(function() \
>> + collectgarbage('collect') \
>> + local f = fio.open(filename, flags, mode) \
>> + fh1 = f.fh \
>> + f = nil \
>> + collectgarbage('collect') \
>> + fiber.yield() \
>> + f = fio.open(filename, flags, mode) \
>> + fh2 = f.fh \
>> + f = nil \
>> + collectgarbage('collect') \
>> + fiber.yield() \
>> + return fh1 == fh2 \
>> +end) or {fh1, fh2}
>> +---
>> +- true
>> +...
>> +-- Ensure, that GC does not break anything after explicit close.
>> +-- Idea of the test is the same as in the previous test, but now
>> +-- the second descriptor is used for something. If GC of the first
>> +-- fio object is called even after close(), it would close the
>> +-- same descriptor, already used by the second fio object. And it
>> +-- won't be able to write anything. Or will write, but to a
>> +-- totally different descriptor created by some other
>> +-- fiber/thread. This is why read() is called on the same file
>> +-- afterwards.
>> +f = nil
>> +---
>> +...
>> +test_run:wait_cond(function() \
>> + f = fio.open(filename, flags, mode) \
>> + fh1 = f.fh \
>> + f:close() \
>> + f = fio.open(filename, flags, mode) \
>> + fh2 = f.fh \
>> + return fh1 == fh2 \
>> +end)
>> +---
>> +- true
>> +...
>> +collectgarbage('collect')
>> +---
>> +- 0
>> +...
>> +fiber.yield()
>> +---
>> +...
>> +f:write('test')
>> +---
>> +- true
>> +...
>> +f:close()
>> +---
>> +- true
>> +...
>> +f = fio.open(filename, flags, mode)
>> +---
>> +...
>> +f:read()
>> +---
>> +- test
>> +...
>> +f:close()
>> +---
>> +- true
>> +...
>> diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
>> index 56c957d8a..c726bade6 100644
>> --- a/test/app/fio.test.lua
>> +++ b/test/app/fio.test.lua
>> @@ -474,3 +474,61 @@ test_run:cmd("clear filter")
>> --
>> fio.mktree('/dev/null')
>> fio.mktree('/dev/null/dir')
>> +
>> +--
>> +-- gh-4727: fio handler GC.
>> +--
>> +flags = {'O_CREAT', 'O_RDWR'}
>> +mode = {'S_IRWXU'}
>> +filename = 'test4727.txt'
>> +fh1 = nil
>> +fh2 = nil
>> +-- Idea of the test is that according to the Open Group standard,
>> +-- open() always returns the smallest available descriptor. This
>> +-- means, that in 'open() + close() + open()' the second open()
>> +-- should return the same value as the first call, if no other
>> +-- threads/fibers managed to interfere. Because of the
>> +-- interference the sequence may need to be called multiple times
>> +-- to catch a couple of equal descriptors.
>> +test_run:wait_cond(function() \
>> + collectgarbage('collect') \
>> + local f = fio.open(filename, flags, mode) \
>> + fh1 = f.fh \
>> + f = nil \
>> + collectgarbage('collect') \
>> +-- GC function of a fio object creates a new fiber. Give it \
>> +-- time to execute. \
>> + fiber.yield() \
>> + f = fio.open(filename, flags, mode) \
>> + fh2 = f.fh \
>> + f = nil \
>> + collectgarbage('collect') \
>> + fiber.yield() \
>> + return fh1 == fh2 \
>> +end) or {fh1, fh2}
>> +
>> +-- Ensure, that GC does not break anything after explicit close.
>> +-- Idea of the test is the same as in the previous test, but now
>> +-- the second descriptor is used for something. If GC of the first
>> +-- fio object is called even after close(), it would close the
>> +-- same descriptor, already used by the second fio object. And it
>> +-- won't be able to write anything. Or will write, but to a
>> +-- totally different descriptor created by some other
>> +-- fiber/thread. This is why read() is called on the same file
>> +-- afterwards.
>> +f = nil
>> +test_run:wait_cond(function() \
>> + f = fio.open(filename, flags, mode) \
>> + fh1 = f.fh \
>> + f:close() \
>> + f = fio.open(filename, flags, mode) \
>> + fh2 = f.fh \
>> + return fh1 == fh2 \
>> +end)
>> +collectgarbage('collect')
>> +fiber.yield()
>> +f:write('test')
>> +f:close()
>> +f = fio.open(filename, flags, mode)
>> +f:read()
>> +f:close()
>
next prev parent reply other threads:[~2020-03-02 20:51 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-09 17:37 Vladislav Shpilevoy
2020-02-09 17:38 ` Vladislav Shpilevoy
2020-02-09 18:34 ` Georgy Kirichenko
2020-02-10 19:36 ` Vladislav Shpilevoy
2020-03-02 20:51 ` Vladislav Shpilevoy [this message]
2020-02-09 19:25 ` Konstantin Osipov
2020-02-10 6:31 ` Kirill Yukhin
2020-02-10 8:20 ` Cyrill Gorcunov
2020-02-10 8:21 ` Cyrill Gorcunov
2020-02-10 19:36 ` Vladislav Shpilevoy
2020-02-14 11:48 ` Kirill Yukhin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=66f6cdbb-03b8-14ac-5b53-58d9919740ee@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=georgy@tarantool.org \
--cc=imun@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox