Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Paul Turner <pjt@google.com>]

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
>