lkml.org 
[lkml]   [2007]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators
    * Fengguang Wu (wfg@mail.ustc.edu.cn) wrote:
    > Al Viro,
    >
    > Does this sounds like a good fix?
    > ===
    >
    > seq_file version fixes
    >
    > - f_version is 'unsigned long', it's pointless to do more than that.

    Hrm, this is weird...

    fs.h:

    struct inode
    u64 i_version;

    and

    struct file
    unsigned long f_version;

    Users do:

    fs/ext3/dir.c:

    if (filp->f_version != inode->i_version) {

    So why isn't f_version a u64 ? It becomes a problem if versions gets
    higher than 2^32 and we are on an architecture where longs are 32 bits.
    I think the problem is the f_version field type, not in seq_file at all.
    I'll prepare a patch for this.

    > - m->version should not be reset when we are bumping up the buf size.
    >

    Hrmmmm, what is this twisted use of versions anyway ?!?

    If I look at other version users elsewhere in the kernel, they mostly
    do:

    repeat:
    f_version = i_version
    do something
    if (f_version != i_version)
    repeat;

    So they can see if the underlying inode has changed during the
    operation. seq_file does it completely the other way around:

    m->version = f_version;
    do something

    and, well, versions are never really used at all.

    If we want to use versioning there, we should keep a version counter
    associated with the ressource pointed used by seq_files that would be
    incremented each time the data structures are modified.

    Then, in the read side, we could sanely do:

    seq open():
    f_version = current version

    seq read():
    repeat:
    m->version = f_version;
    do something
    if (m->version != current version)
    repeat;

    This would only make sure that the given read operation has consistent
    data. It would not certify data consistency across reads.

    I have looked at fs/proc.c/task_mmu.c use of m->version, and I think it
    is just really weird. I think the proper way to do it would be to put
    the last_addr in a field of a structure to which m->private would point
    to.

    Mathieu

    > Signed-off-by: Fengguang Wu <wfg@mail.ustc.edu.cn>
    > ---
    > fs/seq_file.c | 1 -
    > include/linux/seq_file.h | 2 +-
    > 2 files changed, 1 insertion(+), 2 deletions(-)
    >
    > --- linux-2.6.23-rc3.orig/include/linux/seq_file.h
    > +++ linux-2.6.23-rc3/include/linux/seq_file.h
    > @@ -18,7 +18,7 @@ struct seq_file {
    > size_t from;
    > size_t count;
    > loff_t index;
    > - loff_t version;
    > + unsigned long version;
    > struct mutex lock;
    > const struct seq_operations *op;
    > void *private;
    > --- linux-2.6.23-rc3.orig/fs/seq_file.c
    > +++ linux-2.6.23-rc3/fs/seq_file.c
    > @@ -134,7 +134,6 @@ ssize_t seq_read(struct file *file, char
    > if (!m->buf)
    > goto Enomem;
    > m->count = 0;
    > - m->version = 0;
    > }
    > m->op->stop(m, p);
    > m->count = 0;
    >
    > -
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/
    >

    --
    Mathieu Desnoyers
    Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
    OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-08-18 18:27    [W:3.824 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site