[PATCH] sysfs poll should keep the poll rule of normal regular file.

[PATCH] sysfs poll should keep the poll rule of normal regular file.

[KOSAKI Motohiro]

Currently, following test programs don't finished.

% ruby -e '
Thread.new { sleep }
File.read("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies")
'

strace expose the reason.

...
open("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbf9fa6b8) = -1 ENOTTY (Inappropriate ioctl for device)
fstat64(3, {st_mode=S_IFREG|0444, st_size=4096, ...}) = 0
_llseek(3, 0, [0], SEEK_CUR)            = 0
select(4, [3], NULL, NULL, NULL)        = 1 (in [3])
read(3, "1400000 1300000 1200000 1100000 1"..., 4096) = 62
select(4, [3], NULL, NULL, NULL


Because Ruby (the scripting language) VM assume select system-call against regular file don't block.
(POSIX gurantee it.)
But sysfs_poll() don't keep this rule although sysfs file can read and write always.

This patch restore proper poll behavior to sysfs.
/sys/block/md*/md/sync_action polling application and another sysfs updating sensitive
application still can use POLLERR and POLLPRI.



Cc: Neil Brown <neilb@suse.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
--
 fs/sysfs/file.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 289c43a..4a302f8 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -446,11 +446,11 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
 	if (buffer->event != atomic_read(&od->event))
 		goto trigger;
 
-	return 0;
+	return DEFAULT_POLLMASK;
 
  trigger:
 	buffer->needs_read_fill = 1;
-	return POLLERR|POLLPRI;
+	return DEFAULT_POLLMASK|POLLERR|POLLPRI;
 }
 
 void sysfs_notify_dirent(struct sysfs_dirent *sd)

Re: [PATCH] sysfs poll should keep the poll rule of normal regular file.

[Neil Brown]

On Wednesday April 8, kosaki.motohiro@jp.fujitsu.com wrote:
> 
> Currently, following test programs don't finished.
> 
> % ruby -e '
> Thread.new { sleep }
> File.read("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies")
> '
> 
> strace expose the reason.
> 
> ...
> open("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies", O_RDONLY|O_LARGEFILE) = 3
> ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbf9fa6b8) = -1 ENOTTY (Inappropriate ioctl for device)
> fstat64(3, {st_mode=S_IFREG|0444, st_size=4096, ...}) = 0
> _llseek(3, 0, [0], SEEK_CUR)            = 0
> select(4, [3], NULL, NULL, NULL)        = 1 (in [3])
> read(3, "1400000 1300000 1200000 1100000 1"..., 4096) = 62
> select(4, [3], NULL, NULL, NULL
> 
> 
> Because Ruby (the scripting language) VM assume select system-call against regular file don't block.
> (POSIX gurantee it.)
> But sysfs_poll() don't keep this rule although sysfs file can read and write always.

It would be nice to include a reference to where POSIX (or SUS)
guarantees it - though I suspect you are right.

I have one piece of code that this would break, but it isn't released
yet and it is never too late to fix things..

However it should be pointed out that /proc/mounts has exactly the
same problems (/proc/mdstat doesn't - I guess I was lucky enough to
get that one right).  So we should "fix" /proc/mounts
(fs/proc/base.c:mounts_poll) at the same time.  Assuming that won't
break anything.

Al:  do you have an opinion about changing mounts_poll to always
report 'readable' to poll??  What would break?

NeilBrown

> 
> This patch restore proper poll behavior to sysfs.
> /sys/block/md*/md/sync_action polling application and another sysfs updating sensitive
> application still can use POLLERR and POLLPRI.
> 
> 
> 
> Cc: Neil Brown <neilb@suse.de>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> --
>  fs/sysfs/file.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 289c43a..4a302f8 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -446,11 +446,11 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
>  	if (buffer->event != atomic_read(&od->event))
>  		goto trigger;
>  
> -	return 0;
> +	return DEFAULT_POLLMASK;
>  
>   trigger:
>  	buffer->needs_read_fill = 1;
> -	return POLLERR|POLLPRI;
> +	return DEFAULT_POLLMASK|POLLERR|POLLPRI;
>  }
>  
>  void sysfs_notify_dirent(struct sysfs_dirent *sd)
> 

Re: [PATCH] sysfs poll should keep the poll rule of normal regular file.

[KOSAKI Motohiro]

> On Wednesday April 8, kosaki.motohiro@jp.fujitsu.com wrote:
> > 
> > Currently, following test programs don't finished.
> > 
> > % ruby -e '
> > Thread.new { sleep }
> > File.read("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies")
> > '
> > 
> > strace expose the reason.
> > 
> > ...
> > open("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies", O_RDONLY|O_LARGEFILE) = 3
> > ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbf9fa6b8) = -1 ENOTTY (Inappropriate ioctl for device)
> > fstat64(3, {st_mode=S_IFREG|0444, st_size=4096, ...}) = 0
> > _llseek(3, 0, [0], SEEK_CUR)            = 0
> > select(4, [3], NULL, NULL, NULL)        = 1 (in [3])
> > read(3, "1400000 1300000 1200000 1100000 1"..., 4096) = 62
> > select(4, [3], NULL, NULL, NULL
> > 
> > 
> > Because Ruby (the scripting language) VM assume select system-call against regular file don't block.
> > (POSIX gurantee it.)
> > But sysfs_poll() don't keep this rule although sysfs file can read and write always.
> 
> It would be nice to include a reference to where POSIX (or SUS)
> guarantees it - though I suspect you are right.

Oh, very good opinion.

following explanation is better?

poll is requireed "Regular files shall always poll TRUE for reading and
writing." by SUSv3.
  see http://www.opengroup.org/onlinepubs/009695399/functions/poll.html


> I have one piece of code that this would break, but it isn't released
> yet and it is never too late to fix things..
> 
> However it should be pointed out that /proc/mounts has exactly the
> same problems (/proc/mdstat doesn't - I guess I was lucky enough to
> get that one right).  So we should "fix" /proc/mounts
> (fs/proc/base.c:mounts_poll) at the same time.  Assuming that won't
> break anything.
> 
> Al:  do you have an opinion about changing mounts_poll to always
> report 'readable' to poll??  What would break?
> 
> NeilBrown

[PATCH v2 1/2] sysfs poll keep the poll rule of regular file.

[KOSAKI Motohiro]

Currently, following test programs don't finished.

% ruby -e '
Thread.new { sleep }
File.read("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies")
'

strace expose the reason.

...
open("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbf9fa6b8) = -1 ENOTTY (Inappropriate ioctl for device)
fstat64(3, {st_mode=S_IFREG|0444, st_size=4096, ...}) = 0
_llseek(3, 0, [0], SEEK_CUR)            = 0
select(4, [3], NULL, NULL, NULL)        = 1 (in [3])
read(3, "1400000 1300000 1200000 1100000 1"..., 4096) = 62
select(4, [3], NULL, NULL, NULL


Because Ruby (the scripting language) VM assume select system-call against regular file don't block.
it because SUSv3 says "Regular files shall always poll TRUE for reading and writing".
    see http://www.opengroup.org/onlinepubs/009695399/functions/poll.html
it seems valid assumption.

But sysfs_poll() don't keep this rule although sysfs file can read and write always.

This patch restore proper poll behavior to sysfs.
/sys/block/md*/md/sync_action polling application and another sysfs updating sensitive
application still can use POLLERR and POLLPRI.



Cc: Neil Brown <neilb@suse.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
--
 fs/sysfs/file.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 289c43a..4a302f8 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -446,11 +446,11 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
 	if (buffer->event != atomic_read(&od->event))
 		goto trigger;
 
-	return 0;
+	return DEFAULT_POLLMASK;
 
  trigger:
 	buffer->needs_read_fill = 1;
-	return POLLERR|POLLPRI;
+	return DEFAULT_POLLMASK|POLLERR|POLLPRI;
 }
 
 void sysfs_notify_dirent(struct sysfs_dirent *sd)

[PATCH v2 2/2] mounts_poll() make consistent to mdstat_poll

[KOSAKI Motohiro]

In recently sysfs_poll discussion, Neil Brown pointed out /proc/mounts also should
be fixed.

SUSv3 says "Regular files shall always poll TRUE for reading and writing".
  see http://www.opengroup.org/onlinepubs/009695399/functions/poll.html

Then, mounts_poll()'s default should be "POLLIN | POLLRDNORM".
it mean always readable.

In addition, event trigger should use "POLLERR | POLLPRI" instead POLLERR.
it makes consistent to mdstat_poll() and sysfs_poll(). and, select(2) can
handle POLLPRI easily.


Reported-by: Neil Brown <neilb@suse.de>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Miklos Szeredi <mszeredi@suse.cz>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
--
 fs/proc/base.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index f715597..aa763ab 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -648,14 +648,14 @@ static unsigned mounts_poll(struct file *file, poll_table *wait)
 {
 	struct proc_mounts *p = file->private_data;
 	struct mnt_namespace *ns = p->ns;
-	unsigned res = 0;
+	unsigned res = POLLIN | POLLRDNORM;
 
 	poll_wait(file, &ns->poll, wait);
 
 	spin_lock(&vfsmount_lock);
 	if (p->event != ns->event) {
 		p->event = ns->event;
-		res = POLLERR;
+		res |= POLLERR | POLLPRI;
 	}
 	spin_unlock(&vfsmount_lock);
 

Re: [PATCH] sysfs poll should keep the poll rule of normal regular file.

[Neil Brown]

On Thursday April 9, kosaki.motohiro@jp.fujitsu.com wrote:
> > On Wednesday April 8, kosaki.motohiro@jp.fujitsu.com wrote:
> > > Because Ruby (the scripting language) VM assume select system-call against regular file don't block.
> > > (POSIX gurantee it.)
> > > But sysfs_poll() don't keep this rule although sysfs file can read and write always.
> > 
> > It would be nice to include a reference to where POSIX (or SUS)
> > guarantees it - though I suspect you are right.
> 
> Oh, very good opinion.
> 
> following explanation is better?
> 
> poll is requireed "Regular files shall always poll TRUE for reading and
> writing." by SUSv3.
>   see http://www.opengroup.org/onlinepubs/009695399/functions/poll.html
> 

Yes, that's excellent, thanks.

NeilBrown