Bug #4463: [PATCH] release GVL for fcntl() for operations that may block

• Status: Closed
• Assignee: -
• Target version: 2.0.0
• ruby -v: -

[normalperson (Eric Wong)]

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

=end

[kosaki (Motohiro KOSAKI)]

• 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

[kosaki (Motohiro KOSAKI)]

=begin
Hi

2011/3/3 KOSAKI Motohiro [e-mail]:

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

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] 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.

2. 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

[kosaki (Motohiro KOSAKI)]

=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.

2. 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

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] 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

[kosaki (Motohiro KOSAKI)]

• Status changed from Open to Closed

=begin

=end

[normalperson (Eric Wong)]

=begin
Eric Wong [e-mail] wrote:

KOSAKI Motohiro [e-mail] 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

[kosaki (Motohiro KOSAKI)]

=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

[kosaki (Motohiro KOSAKI)]

=begin
Hi

2011/3/3 KOSAKI Motohiro [e-mail]:

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

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] 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.

2. 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

[kosaki (Motohiro KOSAKI)]

=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.

2. 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

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] 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

[normalperson (Eric Wong)]

=begin
Eric Wong [e-mail] wrote:

KOSAKI Motohiro [e-mail] 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

[kosaki (Motohiro KOSAKI)]

=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

[kosaki (Motohiro KOSAKI)]

=begin
Hi

2011/3/3 KOSAKI Motohiro [e-mail]:

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

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] 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.

2. 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

[kosaki (Motohiro KOSAKI)]

=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.

2. 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

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] 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

[normalperson (Eric Wong)]

=begin
Eric Wong [e-mail] wrote:

KOSAKI Motohiro [e-mail] 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