lkml.org 
[lkml]   [2009]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: Migration of kernel interfaces to seq_files breaks pread() consumers
    From
    On Thu, Jan 29, 2009 at 7:01 PM, Eric W. Biederman
    <ebiederm@xmission.com> wrote:
    > Paul Turner <pjt@google.com> writes:
    >>
    >> Thanks Eric,
    >>
    >> Moving the position into the seq_file structure is much cleaner. Basic
    >> tests seem to work ok.
    >
    > Thanks.
    >
    >> Few comments:
    >> - seq_open needs its fmode opened up to take advantage of this [patch
    >> below]
    >
    > Good point.
    > Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
    >
    >
    >> - There's still the inherent problem of reads peturbing other reads (both
    >> read and pread). Our applications should be ok with this, however it does
    >> deviate from the supposedly thread-safe pread semantics.
    >
    > I don't see this not being thread safe.
    >
    > If the data that is returned is stable. We should not see anything
    > changing. Otherwise it is all bets are off in any case.
    >

    With just reads you had stability on the record level at least.
    Concurrent reads/seeks on a file made it obvious where things weren't
    thread safe.

    Obviously userspace doing stupid things isn't a kernel problem but it
    could become one in the future if a previously safe concurrent pread
    interface is migrated to use seqfile.

    > Because seq_read happens under m->lock I don't see how there will
    > be anything thread unsafe.
    >
    > The position that changes is just an internal implementation detail to
    > keep track of which data that we have cached.

    I'm only referring to unstable records since every time we seek we
    trash the cache.

    >
    >>
    >> - Paul
    >>
    >> ---
    >> fs/seq_file.c | 2 --
    >> 1 files changed, 0 insertions(+), 2 deletions(-)
    >>
    >> diff --git a/fs/seq_file.c b/fs/seq_file.c
    >> index cd63d69..aa3621f 100644
    >> --- a/fs/seq_file.c
    >> +++ b/fs/seq_file.c
    >> @@ -48,8 +48,6 @@ int seq_open(struct file *file, const struct seq_operations
    >> *op)
    >> */
    >> file->f_version = 0;
    >>
    >> - /* SEQ files support lseek, but not pread/pwrite */
    >> - file->f_mode &= ~(FMODE_PREAD | FMODE_PWRITE);
    >> return 0;
    >> }
    >> EXPORT_SYMBOL(seq_open);
    >> --
    >> 1.5.4.5
    >


    \
     
     \ /
      Last update: 2009-01-30 07:13    [W:0.025 / U:64.756 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site