[PATCH v2][RESEND] seq_file: don't set read position for invalid iterator

[PATCH v2][RESEND] seq_file: don't set read position for invalid iterator

[Tomasz Majchrzak]

If kernfs file is empty on a first read, successive read operations
using the same file descriptor will return no data, even when data is
available. Default kernfs 'seq_next' implementation advances iterator
position even when next object is not there. Kernfs 'seq_start' for
following requests will not return iterator as position is already on
the second object.

This bug doesn't allow to monitor badblocks sysfs files from MD raid.
They are initially empty but if data appears at some stage, userspace is
not able to read it. It doesn't affect any released applications but it
is necessary for upcoming bad block support for external metadata in MD
raid.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/seq_file.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 6dc4296..74197f4 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -235,7 +235,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 	p = m->op->start(m, &pos);
 	while (1) {
 		err = PTR_ERR(p);
-		if (!p || IS_ERR(p))
+		if (IS_ERR_OR_NULL(p))
 			break;
 		err = m->op->show(m, p);
 		if (err < 0)
@@ -244,7 +244,8 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 			m->count = 0;
 		if (unlikely(!m->count)) {
 			p = m->op->next(m, p, &pos);
-			m->index = pos;
+			if (!IS_ERR_OR_NULL(p))
+				m->index = pos;
 			continue;
 		}
 		if (m->count < m->size)
-- 
1.8.3.1

Re: [PATCH v2][RESEND] seq_file: don't set read position for invalid iterator

[Miklos Szeredi]

On Wed, Oct 12, 2016 at 2:07 PM, Tomasz Majchrzak
<tomasz.majchrzak@intel.com> wrote:
> If kernfs file is empty on a first read, successive read operations
> using the same file descriptor will return no data, even when data is
> available. Default kernfs 'seq_next' implementation advances iterator
> position even when next object is not there. Kernfs 'seq_start' for
> following requests will not return iterator as position is already on
> the second object.
>
> This bug doesn't allow to monitor badblocks sysfs files from MD raid.
> They are initially empty but if data appears at some stage, userspace is
> not able to read it. It doesn't affect any released applications but it
> is necessary for upcoming bad block support for external metadata in MD
> raid.

What is the expectation from the userspace application?  Should
seq_file support "tail -f" as more data is added to the file?  AFAICS
this patch doesn't address that generally, just the empty->nonempty
transition (with the single record case).

Why does userspace app not do open+read+close each time it polls the
badblocks file?

Thanks,
Miklos







>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  fs/seq_file.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 6dc4296..74197f4 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -235,7 +235,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
>         p = m->op->start(m, &pos);
>         while (1) {
>                 err = PTR_ERR(p);
> -               if (!p || IS_ERR(p))
> +               if (IS_ERR_OR_NULL(p))
>                         break;
>                 err = m->op->show(m, p);
>                 if (err < 0)
> @@ -244,7 +244,8 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
>                         m->count = 0;
>                 if (unlikely(!m->count)) {
>                         p = m->op->next(m, p, &pos);
> -                       m->index = pos;
> +                       if (!IS_ERR_OR_NULL(p))
> +                               m->index = pos;
>                         continue;
>                 }
>                 if (m->count < m->size)
> --
> 1.8.3.1
>

Re: [PATCH v2][RESEND] seq_file: don't set read position for invalid iterator

[Tomasz Majchrzak]

On Wed, Oct 26, 2016 at 11:17:13AM +0200, Miklos Szeredi wrote:
> On Wed, Oct 12, 2016 at 2:07 PM, Tomasz Majchrzak
> <tomasz.majchrzak@intel.com> wrote:
> > If kernfs file is empty on a first read, successive read operations
> > using the same file descriptor will return no data, even when data is
> > available. Default kernfs 'seq_next' implementation advances iterator
> > position even when next object is not there. Kernfs 'seq_start' for
> > following requests will not return iterator as position is already on
> > the second object.
> >
> > This bug doesn't allow to monitor badblocks sysfs files from MD raid.
> > They are initially empty but if data appears at some stage, userspace is
> > not able to read it. It doesn't affect any released applications but it
> > is necessary for upcoming bad block support for external metadata in MD
> > raid.
> 
> What is the expectation from the userspace application?  Should
> seq_file support "tail -f" as more data is added to the file?  AFAICS
> this patch doesn't address that generally, just the empty->nonempty
> transition (with the single record case).

Yes, it fixes only empty-nonempty transition. I believe it already works
fine ("tail -f") if the file cursor is in the middle of the file, however
mdmon never performs such operation - it "acknowledges" entry read from the
start of one sysfs file (unacknowledged_bad_blocks) via other sysfs file
(bad_blocks) and reads the sysfs file again from the start (there is new
data at position 0 at this stage).

My patch just resolves a regression introduced by a fix for multiple record
file: 4cdfe84b514 ("deal with the first call of ->show() generating no
output"). It makes it consistent with rest of the code in seq_file
(seq_read, traverse) - new iterator position is ignored if valid iterator
has not been returned.

> Why does userspace app not do open+read+close each time it polls the
> badblocks file?

This excerpt from mdadm/mdmon-design.txt file explains it:

"
The 'monitor' has the primary role of monitoring the array for 
important state changes and updating the metadata accordingly.  As  
writes to the array can be blocked until 'monitor' completes and 
acknowledges the update, it much be very careful not to block itself.
In particular it must not block waiting for any write to complete else
it could deadlock.  This means that it must not allocate memory as
doing this can require dirty memory to be written out and if the 
system choose to write to the array that mdmon is monitoring, the 
memory allocation could deadlock.

So 'monitor' must never allocate memory and must limit the number of
other system call it performs. It may:
- use select (or poll) to wait for activity on a file descriptor
- read from a sysfs file descriptor
- write to a sysfs file descriptor
- write the metadata out to the block devices using O_DIRECT
- send a signal (kill) to the manager thread

It must not e.g. open files or do anything similar that might allocate
resources.
"

Regards,

Tomek

> >
> > Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  fs/seq_file.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > diff --git a/fs/seq_file.c b/fs/seq_file.c
> > index 6dc4296..74197f4 100644
> > --- a/fs/seq_file.c
> > +++ b/fs/seq_file.c
> > @@ -235,7 +235,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
> >         p = m->op->start(m, &pos);
> >         while (1) {
> >                 err = PTR_ERR(p);
> > -               if (!p || IS_ERR(p))
> > +               if (IS_ERR_OR_NULL(p))
> >                         break;
> >                 err = m->op->show(m, p);
> >                 if (err < 0)
> > @@ -244,7 +244,8 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
> >                         m->count = 0;
> >                 if (unlikely(!m->count)) {
> >                         p = m->op->next(m, p, &pos);
> > -                       m->index = pos;
> > +                       if (!IS_ERR_OR_NULL(p))
> > +                               m->index = pos;
> >                         continue;
> >                 }
> >                 if (m->count < m->size)
> > --
> > 1.8.3.1
> >

Re: [PATCH v2][RESEND] seq_file: don't set read position for invalid iterator

[Miklos Szeredi]

On Mon, Oct 31, 2016 at 10:32:21AM +0100, Tomasz Majchrzak wrote:
> On Wed, Oct 26, 2016 at 11:17:13AM +0200, Miklos Szeredi wrote:
> > On Wed, Oct 12, 2016 at 2:07 PM, Tomasz Majchrzak
> > <tomasz.majchrzak@intel.com> wrote:
> > > If kernfs file is empty on a first read, successive read operations
> > > using the same file descriptor will return no data, even when data is
> > > available. Default kernfs 'seq_next' implementation advances iterator
> > > position even when next object is not there. Kernfs 'seq_start' for
> > > following requests will not return iterator as position is already on
> > > the second object.
> > >
> > > This bug doesn't allow to monitor badblocks sysfs files from MD raid.
> > > They are initially empty but if data appears at some stage, userspace is
> > > not able to read it. It doesn't affect any released applications but it
> > > is necessary for upcoming bad block support for external metadata in MD
> > > raid.
> > 
> > What is the expectation from the userspace application?  Should
> > seq_file support "tail -f" as more data is added to the file?  AFAICS
> > this patch doesn't address that generally, just the empty->nonempty
> > transition (with the single record case).
> 
> Yes, it fixes only empty-nonempty transition. I believe it already works
> fine ("tail -f") if the file cursor is in the middle of the file,

"tail -f" works if there are multiple records.  It doesn't work for the "single"
style of seq_file used by sysfs.

> however
> mdmon never performs such operation - it "acknowledges" entry read from the
> start of one sysfs file (unacknowledged_bad_blocks) via other sysfs file
> (bad_blocks) and reads the sysfs file again from the start (there is new
> data at position 0 at this stage).
> 
> My patch just resolves a regression introduced by a fix for multiple record
> file: 4cdfe84b514 ("deal with the first call of ->show() generating no
> output").

The above commit (from 2.6.27) didn't change the beavior relative to sysfs
files, AFAICS.  Before that commit the index was incremented if the first
iterator was empty, just like after the patch.

>  It makes it consistent with rest of the code in seq_file
> (seq_read, traverse) - new iterator position is ignored if valid iterator
> has not been returned.

Your patch doesn't seem to make the behavior consistent, it just special cases
the last record being empty.  What if the last two (three, etc) records are
empty?  It would be consistent if m->index always pointed to the first record
matching the given file position, for example.

But I doubt that we actually need to do that.  For example just special casing
the zero offset (always translating to zero index) would be conceptually simpler
and equivalent to your patch for the sysfs case.

But see below.  I still don't see what you gain by not doing the open/read/close
when polling the badblocks file.

> > Why does userspace app not do open+read+close each time it polls the
> > badblocks file?
> 
> This excerpt from mdadm/mdmon-design.txt file explains it:
> 
> "
> The 'monitor' has the primary role of monitoring the array for 
> important state changes and updating the metadata accordingly.  As  
> writes to the array can be blocked until 'monitor' completes and 
> acknowledges the update, it much be very careful not to block itself.
> In particular it must not block waiting for any write to complete else
> it could deadlock.  This means that it must not allocate memory as
> doing this can require dirty memory to be written out and if the 
> system choose to write to the array that mdmon is monitoring, the 
> memory allocation could deadlock.
> 
> So 'monitor' must never allocate memory and must limit the number of
> other system call it performs. It may:
> - use select (or poll) to wait for activity on a file descriptor
> - read from a sysfs file descriptor
> - write to a sysfs file descriptor
> - write the metadata out to the block devices using O_DIRECT
> - send a signal (kill) to the manager thread
> 
> It must not e.g. open files or do anything similar that might allocate
> resources.
> "

seq_read() *will* allocate memory; see seq_buf_alloc().

Thanks,
Miklos

Re: [PATCH v2][RESEND] seq_file: don't set read position for invalid iterator

[Tomasz Majchrzak]

On Wed, Nov 02, 2016 at 10:11:47AM +0100, Miklos Szeredi wrote:
> On Mon, Oct 31, 2016 at 10:32:21AM +0100, Tomasz Majchrzak wrote:
> > On Wed, Oct 26, 2016 at 11:17:13AM +0200, Miklos Szeredi wrote:
> > > On Wed, Oct 12, 2016 at 2:07 PM, Tomasz Majchrzak
> > > <tomasz.majchrzak@intel.com> wrote:
> > > > If kernfs file is empty on a first read, successive read operations
> > > > using the same file descriptor will return no data, even when data is
> > > > available. Default kernfs 'seq_next' implementation advances iterator
> > > > position even when next object is not there. Kernfs 'seq_start' for
> > > > following requests will not return iterator as position is already on
> > > > the second object.
> > > >
> > > > This bug doesn't allow to monitor badblocks sysfs files from MD raid.
> > > > They are initially empty but if data appears at some stage, userspace is
> > > > not able to read it. It doesn't affect any released applications but it
> > > > is necessary for upcoming bad block support for external metadata in MD
> > > > raid.
> > > 
> > > What is the expectation from the userspace application?  Should
> > > seq_file support "tail -f" as more data is added to the file?  AFAICS
> > > this patch doesn't address that generally, just the empty->nonempty
> > > transition (with the single record case).
> > 
> > Yes, it fixes only empty-nonempty transition. I believe it already works
> > fine ("tail -f") if the file cursor is in the middle of the file,
> 
> "tail -f" works if there are multiple records.  It doesn't work for the "single"
> style of seq_file used by sysfs.
> 
> > however
> > mdmon never performs such operation - it "acknowledges" entry read from the
> > start of one sysfs file (unacknowledged_bad_blocks) via other sysfs file
> > (bad_blocks) and reads the sysfs file again from the start (there is new
> > data at position 0 at this stage).
> > 
> > My patch just resolves a regression introduced by a fix for multiple record
> > file: 4cdfe84b514 ("deal with the first call of ->show() generating no
> > output").
> 
> The above commit (from 2.6.27) didn't change the beavior relative to sysfs
> files, AFAICS.  Before that commit the index was incremented if the first
> iterator was empty, just like after the patch.

After the patch m->index is updated after 'next' iterator, regardless if valid
or invalid iterator has been returned. Prior to the patch there was no call to
'next' operator at all. The similar code few lines below ignores position
for invalid iterator.

> >  It makes it consistent with rest of the code in seq_file
> > (seq_read, traverse) - new iterator position is ignored if valid iterator
> > has not been returned.
> 
> Your patch doesn't seem to make the behavior consistent, it just special cases
> the last record being empty.  What if the last two (three, etc) records are
> empty?  It would be consistent if m->index always pointed to the first record
> matching the given file position, for example.

I don't understand it. Is it possible to map file position to the index
(record)? I think there is no way to determine record size without actually
reading it.

> But I doubt that we actually need to do that.  For example just special casing
> the zero offset (always translating to zero index) would be conceptually simpler
> and equivalent to your patch for the sysfs case.

Do you mean such piece of code?

if (ppos == 0)
	m->index = 0;

> But see below.  I still don't see what you gain by not doing the open/read/close
> when polling the badblocks file.
> 
> > > Why does userspace app not do open+read+close each time it polls the
> > > badblocks file?
> > 
> > This excerpt from mdadm/mdmon-design.txt file explains it:
> > 
> > "
> > The 'monitor' has the primary role of monitoring the array for 
> > important state changes and updating the metadata accordingly.  As  
> > writes to the array can be blocked until 'monitor' completes and 
> > acknowledges the update, it much be very careful not to block itself.
> > In particular it must not block waiting for any write to complete else
> > it could deadlock.  This means that it must not allocate memory as
> > doing this can require dirty memory to be written out and if the 
> > system choose to write to the array that mdmon is monitoring, the 
> > memory allocation could deadlock.
> > 
> > So 'monitor' must never allocate memory and must limit the number of
> > other system call it performs. It may:
> > - use select (or poll) to wait for activity on a file descriptor
> > - read from a sysfs file descriptor
> > - write to a sysfs file descriptor
> > - write the metadata out to the block devices using O_DIRECT
> > - send a signal (kill) to the manager thread
> > 
> > It must not e.g. open files or do anything similar that might allocate
> > resources.
> > "
> 
> seq_read() *will* allocate memory; see seq_buf_alloc().

mdmon (RAID monitoring application) opens and reads sysfs file during
initialization stage. At this stage RAID array is not enabled yet so memory
allocations are allowed. The first read doesn't process the data obtained,
it is just performed in order to trigger memory allocation. The file is not
being closed later in order to keep the buffer. The sysfs files used in such
manner are single-record and their length do not exceed page size so no
additional memory allocations are performed in next read operations.

Tomek

Re: [PATCH v2][RESEND] seq_file: don't set read position for invalid iterator

[Miklos Szeredi]

On Thu, Nov 24, 2016 at 4:23 PM, Tomasz Majchrzak
<tomasz.majchrzak@intel.com> wrote:

> I don't understand it. Is it possible to map file position to the index
> (record)? I think there is no way to determine record size without actually
> reading it.

Right.  So don't update m->index when a  zero size record is read.
This will result in m->index always pointing to the first record at
the given offset.  You use this property for the single record case
and zero offset, but I'd consider behavior consistent if this was not
just done for this special case.

>> But I doubt that we actually need to do that.  For example just special casing
>> the zero offset (always translating to zero index) would be conceptually simpler
>> and equivalent to your patch for the sysfs case.
>
> Do you mean such piece of code?
>
> if (ppos == 0)
>         m->index = 0;

Yes.  Which is a special case, but it's at least simple and easy to prove right.

Thanks,
Miklos

[PATCH v3] seq_file: reset iterator to first record for zero offset

[Tomasz Majchrzak]

If kernfs file is empty on a first read, successive read operations
using the same file descriptor will return no data, even when data is
available. Default kernfs 'seq_next' implementation advances iterator
position even when next object is not there. Kernfs 'seq_start' for
following requests will not return iterator as position is already on
the second object.

This defect doesn't allow to monitor badblocks sysfs files from MD raid.
They are initially empty but if data appears at some stage, userspace is
not able to read it.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 fs/seq_file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 368bfb9..ee21714 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -190,6 +190,9 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 	 */
 	m->version = file->f_version;
 
+	if (*ppos == 0)
+		m->index = 0;
+
 	/* Don't assume *ppos is where we left it */
 	if (unlikely(*ppos != m->read_pos)) {
 		while ((err = traverse(m, *ppos)) == -EAGAIN)
-- 
1.8.3.1

Re: [PATCH v3] seq_file: reset iterator to first record for zero offset

[Miklos Szeredi]

On Mon, Nov 28, 2016 at 4:11 PM, Tomasz Majchrzak
<tomasz.majchrzak@intel.com> wrote:
> If kernfs file is empty on a first read, successive read operations
> using the same file descriptor will return no data, even when data is
> available. Default kernfs 'seq_next' implementation advances iterator
> position even when next object is not there. Kernfs 'seq_start' for
> following requests will not return iterator as position is already on
> the second object.
>
> This defect doesn't allow to monitor badblocks sysfs files from MD raid.
> They are initially empty but if data appears at some stage, userspace is
> not able to read it.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>

A comment above the check wouldn't hurt.  Otherwise

Acked-by: Miklos Szeredi <mszeredi@redhat.com>

> ---
>  fs/seq_file.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 368bfb9..ee21714 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -190,6 +190,9 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
>          */
>         m->version = file->f_version;
>
> +       if (*ppos == 0)
> +               m->index = 0;
> +
>         /* Don't assume *ppos is where we left it */
>         if (unlikely(*ppos != m->read_pos)) {
>                 while ((err = traverse(m, *ppos)) == -EAGAIN)
> --
> 1.8.3.1
>

[PATCH v4] seq_file: reset iterator to first record for zero offset

[Tomasz Majchrzak]

If kernfs file is empty on a first read, successive read operations
using the same file descriptor will return no data, even when data is
available. Default kernfs 'seq_next' implementation advances iterator
position even when next object is not there. Kernfs 'seq_start' for
following requests will not return iterator as position is already on
the second object.

This defect doesn't allow to monitor badblocks sysfs files from MD raid.
They are initially empty but if data appears at some stage, userspace is
not able to read it.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Acked-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/seq_file.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 368bfb9..a11f271 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -190,6 +190,13 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 	 */
 	m->version = file->f_version;
 
+	/*
+	 * if request is to read from zero offset, reset iterator to first
+	 * record as it might have been already advanced by previous requests
+	 */
+	if (*ppos == 0)
+		m->index = 0;
+
 	/* Don't assume *ppos is where we left it */
 	if (unlikely(*ppos != m->read_pos)) {
 		while ((err = traverse(m, *ppos)) == -EAGAIN)
-- 
1.8.3.1