Project

General

Profile

Actions

Bug #4463

closed

[PATCH] release GVL for fcntl() for operations that may block

Added by normalperson (Eric Wong) about 13 years ago. Updated almost 13 years ago.

Status:
Closed
Assignee:
-
Target version:
ruby -v:
-
Backport:
[ruby-core:35417]

Description

=begin
Users of F_SETLKW may block the entire VM via IO#fcntl,
release the GVL so other operations may continue.

=end


Files

Updated by kosaki (Motohiro KOSAKI) about 13 years ago

  • ruby -v changed from ruby 1.9.3dev (2011-03-03 trunk 31011) [x86_64-linux] to -

=begin

Issue #4463 has been reported by Eric Wong.


Bug #4463: [PATCH] release GVL for fcntl() for operations that may block
http://redmine.ruby-lang.org/issues/4463

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-03-03 trunk 31011) [x86_64-linux]

Users of F_SETLKW may block the entire VM via IO#fcntl,
release the GVL so other operations may continue.

Yeah.
It looks reasonable request. :)
=end

Updated by kosaki (Motohiro KOSAKI) about 13 years ago

=begin
Hi

2011/3/3 KOSAKI Motohiro :

Issue #4463 has been reported by Eric Wong.


Bug #4463: [PATCH] release GVL for fcntl() for operations that may block
http://redmine.ruby-lang.org/issues/4463

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-03-03 trunk 31011) [x86_64-linux]

Users of F_SETLKW may block the entire VM via IO#fcntl,
release the GVL so other operations may continue.

Yeah.
It looks reasonable request. :)

Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
    A) if a user are using network filesystem, almost all fcntl need network
    communication. iow, they can be blocked.
    B) We are sure ioctl() has similar issue. But, we don't have any knowledge
    which ioctl can be blocked. It is strongly dependend a
    platform and a device.
  2. Added small test. It is based on your Fcntl::Flock patch.

Thanks.
=end

Updated by normalperson (Eric Wong) about 13 years ago

=begin
KOSAKI Motohiro wrote:

Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
    A) if a user are using network filesystem, almost all fcntl need network
    communication. iow, they can be blocked.
    B) We are sure ioctl() has similar issue. But, we don't have any knowledge
    which ioctl can be blocked. It is strongly dependend a
    platform and a device.

Agreed on both points.

  1. Added small test. It is based on your Fcntl::Flock patch.

Any chance of that patch making it into trunk? I'd be happy to make
any changes/improvements necessary (+docs, too). Thanks again.

--
Eric Wong
=end

Updated by kosaki (Motohiro KOSAKI) about 13 years ago

=begin
Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
        A) if a user are using network filesystem, almost all fcntl need network
            communication. iow, they can be blocked.
        B) We are sure ioctl() has similar issue. But, we don't have any knowledge
            which ioctl can be blocked. It is strongly dependend a
    platform and a device.

Agreed on both points.

thank you.

  1. Added small test. It is based on your Fcntl::Flock patch.

Any chance of that patch making it into trunk?  I'd be happy to make
any changes/improvements necessary (+docs, too).  Thanks again.

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

   lock

=end

Updated by normalperson (Eric Wong) about 13 years ago

=begin
KOSAKI Motohiro wrote:

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

  lock = Fcntl::Flock.new Fcntl::F_WRLCK
  f.fcntl Fcntl::F_SETLKW, lock

I agree it's currently too verbose.

I tried to keep io.c the same so I used a String subclass. Maybe I
should just modify teach io.c to deal with Hash/Array arguments? I do
worry about placing more burden on io.c for portability reasons, though
POSIX file locks might be very common by now...

To shorten interface, maybe Fcntl::Flock[] can return an array for splat
and take symbol args (like new Socket):

f.fcntl *Fcntl::Flock[:F_SETLKW, :F_WRLCK, :SEEK_SET, 0, 0]

Or maybe even:

f.fcntl *Fcntl::Flock[:SETLKW, :WRLCK, :SET, 0, 0]

but I personally prefer array or hash capsulation. e.g

f.fcntl Fcntl:F_SETLKW, [Fcntl:F_WRLK, SEEK_SET, 0, 0]

or

f.fcntl Fcntl:F_SETLKW, { :l_type => Fcntl:F_WRLK }

Yes, I like the Hash one but requires modifying io.c with potentially
unportable code to support.

If we use non-String, maybe just call fcntl(2) inside ext/fcntl/fcntl.c
internally and forget about IO#fcntl in io.c entirely:

Fcntl::Flock[:WRLCK, :SET, 0, 0].lock(io)
Fcntl::Flock[:WRLCK, :SET, 0, 0].try_lock(io)
Fcntl::Flock[:SET, 0, 0].unlock(io)

Or even:

Fcntl.lock(io, :WRLCK, :SET, 0, 0)
Fcntl.try_lock(io, :WRLCK, :SET, 0, 0)
Fcntl.unlock(io, :SET, 0, 0)
Fcntl.getlock(io, :RDLCK, :SET, 0, 0) -> Fcntl::Flock object

That would allow us to do something stateful like:

Fcntl.synchronize(io, :WRLCK, :SET, 0, 0) do
  # ...
end

I dislike all caps, even, taking hints from pthread_rwlock_*:

Fcntl.rdlock(io, :set, 0, 0)
Fcntl.tryrdlock(io, :set, 0, 0)
Fcntl.wrlock(io, :set, 0, 0)
Fcntl.trywrlock(io, :set, 0, 0)
Fcntl.unlock(io, :set, 0, 0)

Fcntl.read_synchronize(io, :set, 0, 0) do
  # ...
end

Fcntl.synchronize(io, :set, 0, 0) do
  # ...
end

But, of cource, I'm not against if matz ack yours. So I recommend you
describe the detailed interface to matz instead only just attached a patch.
It's best practice to persuade very busy person. :)

Thanks again for the feedback. So many ways to do this interface,
but just anything but Array#pack sounds good to me :)

--
Eric Wong
=end

Updated by kosaki (Motohiro KOSAKI) about 13 years ago

  • Status changed from Open to Closed

=begin

=end

Updated by normalperson (Eric Wong) about 13 years ago

=begin
Eric Wong wrote:

KOSAKI Motohiro wrote:

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

That would allow us to do something stateful like:

Fcntl.synchronize(io, :WRLCK, :SET, 0, 0) do

...

end

Following up, I went with something along these lines here.

http://redmine.ruby-lang.org/issues/4464
http://redmine.ruby-lang.org/attachments/1540/0001-add-Fcntl-Lock-object-for-easier-use-of-POSIX-file-l.patch

Simple use case to lock the whole file is just:

Fcntl::Lock.synchronize(file) do
# ...
end

--
Eric Wong
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin

Issue #4463 has been reported by Eric Wong.


Bug #4463: [PATCH] release GVL for fcntl() for operations that may block
http://redmine.ruby-lang.org/issues/4463

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-03-03 trunk 31011) [x86_64-linux]

Users of F_SETLKW may block the entire VM via IO#fcntl,
release the GVL so other operations may continue.

Yeah.
It looks reasonable request. :)
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin
Hi

2011/3/3 KOSAKI Motohiro :

Issue #4463 has been reported by Eric Wong.


Bug #4463: [PATCH] release GVL for fcntl() for operations that may block
http://redmine.ruby-lang.org/issues/4463

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-03-03 trunk 31011) [x86_64-linux]

Users of F_SETLKW may block the entire VM via IO#fcntl,
release the GVL so other operations may continue.

Yeah.
It looks reasonable request. :)

Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
    A) if a user are using network filesystem, almost all fcntl need network
    communication. iow, they can be blocked.
    B) We are sure ioctl() has similar issue. But, we don't have any knowledge
    which ioctl can be blocked. It is strongly dependend a
    platform and a device.
  2. Added small test. It is based on your Fcntl::Flock patch.

Thanks.
=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
KOSAKI Motohiro wrote:

Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
    A) if a user are using network filesystem, almost all fcntl need network
    communication. iow, they can be blocked.
    B) We are sure ioctl() has similar issue. But, we don't have any knowledge
    which ioctl can be blocked. It is strongly dependend a
    platform and a device.

Agreed on both points.

  1. Added small test. It is based on your Fcntl::Flock patch.

Any chance of that patch making it into trunk? I'd be happy to make
any changes/improvements necessary (+docs, too). Thanks again.

--
Eric Wong
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin
Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
        A) if a user are using network filesystem, almost all fcntl need network
            communication. iow, they can be blocked.
        B) We are sure ioctl() has similar issue. But, we don't have any knowledge
            which ioctl can be blocked. It is strongly dependend a
    platform and a device.

Agreed on both points.

thank you.

  1. Added small test. It is based on your Fcntl::Flock patch.

Any chance of that patch making it into trunk?  I'd be happy to make
any changes/improvements necessary (+docs, too).  Thanks again.

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

   lock

=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
KOSAKI Motohiro wrote:

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

  lock = Fcntl::Flock.new Fcntl::F_WRLCK
  f.fcntl Fcntl::F_SETLKW, lock

I agree it's currently too verbose.

I tried to keep io.c the same so I used a String subclass. Maybe I
should just modify teach io.c to deal with Hash/Array arguments? I do
worry about placing more burden on io.c for portability reasons, though
POSIX file locks might be very common by now...

To shorten interface, maybe Fcntl::Flock[] can return an array for splat
and take symbol args (like new Socket):

f.fcntl *Fcntl::Flock[:F_SETLKW, :F_WRLCK, :SEEK_SET, 0, 0]

Or maybe even:

f.fcntl *Fcntl::Flock[:SETLKW, :WRLCK, :SET, 0, 0]

but I personally prefer array or hash capsulation. e.g

f.fcntl Fcntl:F_SETLKW, [Fcntl:F_WRLK, SEEK_SET, 0, 0]

or

f.fcntl Fcntl:F_SETLKW, { :l_type => Fcntl:F_WRLK }

Yes, I like the Hash one but requires modifying io.c with potentially
unportable code to support.

If we use non-String, maybe just call fcntl(2) inside ext/fcntl/fcntl.c
internally and forget about IO#fcntl in io.c entirely:

Fcntl::Flock[:WRLCK, :SET, 0, 0].lock(io)
Fcntl::Flock[:WRLCK, :SET, 0, 0].try_lock(io)
Fcntl::Flock[:SET, 0, 0].unlock(io)

Or even:

Fcntl.lock(io, :WRLCK, :SET, 0, 0)
Fcntl.try_lock(io, :WRLCK, :SET, 0, 0)
Fcntl.unlock(io, :SET, 0, 0)
Fcntl.getlock(io, :RDLCK, :SET, 0, 0) -> Fcntl::Flock object

That would allow us to do something stateful like:

Fcntl.synchronize(io, :WRLCK, :SET, 0, 0) do
  # ...
end

I dislike all caps, even, taking hints from pthread_rwlock_*:

Fcntl.rdlock(io, :set, 0, 0)
Fcntl.tryrdlock(io, :set, 0, 0)
Fcntl.wrlock(io, :set, 0, 0)
Fcntl.trywrlock(io, :set, 0, 0)
Fcntl.unlock(io, :set, 0, 0)

Fcntl.read_synchronize(io, :set, 0, 0) do
  # ...
end

Fcntl.synchronize(io, :set, 0, 0) do
  # ...
end

But, of cource, I'm not against if matz ack yours. So I recommend you
describe the detailed interface to matz instead only just attached a patch.
It's best practice to persuade very busy person. :)

Thanks again for the feedback. So many ways to do this interface,
but just anything but Array#pack sounds good to me :)

--
Eric Wong
=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
Eric Wong wrote:

KOSAKI Motohiro wrote:

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

That would allow us to do something stateful like:

Fcntl.synchronize(io, :WRLCK, :SET, 0, 0) do

...

end

Following up, I went with something along these lines here.

http://redmine.ruby-lang.org/issues/4464
http://redmine.ruby-lang.org/attachments/1540/0001-add-Fcntl-Lock-object-for-easier-use-of-POSIX-file-l.patch

Simple use case to lock the whole file is just:

Fcntl::Lock.synchronize(file) do
# ...
end

--
Eric Wong
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin

Issue #4463 has been reported by Eric Wong.


Bug #4463: [PATCH] release GVL for fcntl() for operations that may block
http://redmine.ruby-lang.org/issues/4463

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-03-03 trunk 31011) [x86_64-linux]

Users of F_SETLKW may block the entire VM via IO#fcntl,
release the GVL so other operations may continue.

Yeah.
It looks reasonable request. :)
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin
Hi

2011/3/3 KOSAKI Motohiro :

Issue #4463 has been reported by Eric Wong.


Bug #4463: [PATCH] release GVL for fcntl() for operations that may block
http://redmine.ruby-lang.org/issues/4463

Author: Eric Wong
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-03-03 trunk 31011) [x86_64-linux]

Users of F_SETLKW may block the entire VM via IO#fcntl,
release the GVL so other operations may continue.

Yeah.
It looks reasonable request. :)

Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
    A) if a user are using network filesystem, almost all fcntl need network
    communication. iow, they can be blocked.
    B) We are sure ioctl() has similar issue. But, we don't have any knowledge
    which ioctl can be blocked. It is strongly dependend a
    platform and a device.
  2. Added small test. It is based on your Fcntl::Flock patch.

Thanks.
=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
KOSAKI Motohiro wrote:

Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
    A) if a user are using network filesystem, almost all fcntl need network
    communication. iow, they can be blocked.
    B) We are sure ioctl() has similar issue. But, we don't have any knowledge
    which ioctl can be blocked. It is strongly dependend a
    platform and a device.

Agreed on both points.

  1. Added small test. It is based on your Fcntl::Flock patch.

Any chance of that patch making it into trunk? I'd be happy to make
any changes/improvements necessary (+docs, too). Thanks again.

--
Eric Wong
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin
Hi

I've commited slightly modified version today (r31025).
The difference is,

  1. All IO.fcntl() and IO.iocntl() relese GVL instead only SETLCKW. because,
        A) if a user are using network filesystem, almost all fcntl need network
            communication. iow, they can be blocked.
        B) We are sure ioctl() has similar issue. But, we don't have any knowledge
            which ioctl can be blocked. It is strongly dependend a
    platform and a device.

Agreed on both points.

thank you.

  1. Added small test. It is based on your Fcntl::Flock patch.

Any chance of that patch making it into trunk?  I'd be happy to make
any changes/improvements necessary (+docs, too).  Thanks again.

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

   lock

=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
KOSAKI Motohiro wrote:

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

  lock = Fcntl::Flock.new Fcntl::F_WRLCK
  f.fcntl Fcntl::F_SETLKW, lock

I agree it's currently too verbose.

I tried to keep io.c the same so I used a String subclass. Maybe I
should just modify teach io.c to deal with Hash/Array arguments? I do
worry about placing more burden on io.c for portability reasons, though
POSIX file locks might be very common by now...

To shorten interface, maybe Fcntl::Flock[] can return an array for splat
and take symbol args (like new Socket):

f.fcntl *Fcntl::Flock[:F_SETLKW, :F_WRLCK, :SEEK_SET, 0, 0]

Or maybe even:

f.fcntl *Fcntl::Flock[:SETLKW, :WRLCK, :SET, 0, 0]

but I personally prefer array or hash capsulation. e.g

f.fcntl Fcntl:F_SETLKW, [Fcntl:F_WRLK, SEEK_SET, 0, 0]

or

f.fcntl Fcntl:F_SETLKW, { :l_type => Fcntl:F_WRLK }

Yes, I like the Hash one but requires modifying io.c with potentially
unportable code to support.

If we use non-String, maybe just call fcntl(2) inside ext/fcntl/fcntl.c
internally and forget about IO#fcntl in io.c entirely:

Fcntl::Flock[:WRLCK, :SET, 0, 0].lock(io)
Fcntl::Flock[:WRLCK, :SET, 0, 0].try_lock(io)
Fcntl::Flock[:SET, 0, 0].unlock(io)

Or even:

Fcntl.lock(io, :WRLCK, :SET, 0, 0)
Fcntl.try_lock(io, :WRLCK, :SET, 0, 0)
Fcntl.unlock(io, :SET, 0, 0)
Fcntl.getlock(io, :RDLCK, :SET, 0, 0) -> Fcntl::Flock object

That would allow us to do something stateful like:

Fcntl.synchronize(io, :WRLCK, :SET, 0, 0) do
  # ...
end

I dislike all caps, even, taking hints from pthread_rwlock_*:

Fcntl.rdlock(io, :set, 0, 0)
Fcntl.tryrdlock(io, :set, 0, 0)
Fcntl.wrlock(io, :set, 0, 0)
Fcntl.trywrlock(io, :set, 0, 0)
Fcntl.unlock(io, :set, 0, 0)

Fcntl.read_synchronize(io, :set, 0, 0) do
  # ...
end

Fcntl.synchronize(io, :set, 0, 0) do
  # ...
end

But, of cource, I'm not against if matz ack yours. So I recommend you
describe the detailed interface to matz instead only just attached a patch.
It's best practice to persuade very busy person. :)

Thanks again for the feedback. So many ways to do this interface,
but just anything but Array#pack sounds good to me :)

--
Eric Wong
=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
Eric Wong wrote:

KOSAKI Motohiro wrote:

Umm..
I don't like its interface so much. your flock object don't mange any lock
state. it's merely wrapper of argument of fcntl. your interface mean we need
two line every lock operation. eg.

That would allow us to do something stateful like:

Fcntl.synchronize(io, :WRLCK, :SET, 0, 0) do

...

end

Following up, I went with something along these lines here.

http://redmine.ruby-lang.org/issues/4464
http://redmine.ruby-lang.org/attachments/1540/0001-add-Fcntl-Lock-object-for-easier-use-of-POSIX-file-l.patch

Simple use case to lock the whole file is just:

Fcntl::Lock.synchronize(file) do
# ...
end

--
Eric Wong
=end

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0