lkml.org 
[lkml]   [2008]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RESEND] [PATCH] VFS: make file->f_pos access atomic on 32bit arch
    On Wed, Oct 08, 2008 at 03:27:44AM +1100, Nick Piggin wrote:
    > So.. is everyone agreed that corrupting f_pos is a bad thing? (serious
    > question) If so, then we should get something like this merged sooner
    > rather than later.

    I'm not convinced it's a bad thing. f_pos is going to get 'corrupted'
    (ie not pointing where you think it's going to point) sooner or later
    anyway if you have two threads accessing the same file without locking.
    The fact that it can point to an offset that was accessed by neither
    thread A nor thread B seems irrelevant to me.


    Separately, if we do decide something along these lines is a good idea,
    then file_pos_update() needs to be better. The current patch has:

    if (offset>=0 && offset<=inode->i_sb->s_maxbytes) {
    /* Special lock needed here? */
    - if (offset != file->f_pos) {
    - file->f_pos = offset;
    + if (file_pos_update(file, offset))
    file->f_version = 0;
    - }
    retval = offset;
    }

    It seems to me that the set of f_version to 0 should be atomic with
    the changing of f_pos. So something like this might work better:

    +static inline void file_pos_update(struct file *file, loff_t offset, int v)
    +{
    +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
    + write_seqlock(&file->f_pos_seqlock);
    + if (offset != file->f_pos) {
    + file->f_pos = offset;
    + if (v)
    + file->f_version = 0;
    + }
    + write_sequnlock(&file->f_pos_seqlock);
    +#elif BITS_PER_LONG == 32 && defined(CONFIG_PREEMPT)
    + preempt_disable();
    + if (offset != file->f_pos) {
    + file->f_pos = offset;
    + if (v)
    + file->f_version = 0;
    + }
    + preempt_enable();
    +#else
    + if (offset != file->f_pos) {
    + file->f_pos = offset;
    + if (v)
    + file->f_version = 0;
    + }
    +#endif
    +}

    Since 'v' will be known at compile time, the tests optimise away to nothing.

    I also think the patch would be more favourably received if it were
    structured as:

    +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
    +static inline loff_t file_pos_read(struct file *file)
    +{
    ...
    +}
    +
    +static inline void file_pos_write(struct file *file, loff_t pos)
    +{
    ...
    +}
    +
    ...
    +#elif BITS_PER_LONG == 32 && defined(CONFIG_PREEMPT)
    ...
    +#else
    ...
    +#endif


    --
    Matthew Wilcox Intel Open Source Technology Centre
    "Bill, look, we understand that you're interested in selling us this
    operating system, but compare it to ours. We can't possibly take such
    a retrograde step."


    \
     
     \ /
      Last update: 2008-10-07 20:03    [W:0.029 / U:123.896 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site