lkml.org 
[lkml]   [2009]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 6/8] exofs: super_operations and file_system_type
    Hi.

    On Mon, Feb 09, 2009 at 03:25:53PM +0200, Boaz Harrosh (bharrosh@panasas.com) wrote:
    > +static int parse_options(char *options, struct exofs_mountopt *opts)
    > +{
    > + char *p;
    > + substring_t args[MAX_OPT_ARGS];
    > + int option;
    > + bool s_pid = false;
    > +
    > + EXOFS_DBGMSG("parse_options %s\n", options);
    > + /* defaults */
    > + memset(opts, 0, sizeof(*opts));
    > + opts->timeout = BLK_DEFAULT_SG_TIMEOUT;
    > +
    > + while ((p = strsep(&options, ",")) != NULL) {
    > + int token;
    > + char str[32];
    > +
    > + if (!*p)
    > + continue;
    > +
    > + token = match_token(p, tokens, args);
    > + switch (token) {
    > + case Opt_pid:
    > + if (0 == match_strlcpy(str, &args[0], sizeof(str)))
    > + return -EINVAL;
    > + opts->pid = simple_strtoull(str, NULL, 0);
    > + if (opts->pid < EXOFS_MIN_PID) {
    > + EXOFS_ERR("Partition ID must be >= %u",
    > + EXOFS_MIN_PID);
    > + return -EINVAL;
    > + }
    > + s_pid = 1;
    > + break;
    > + case Opt_to:
    > + if (match_int(&args[0], &option))
    > + return -EINVAL;
    > + if (option <= 0) {
    > + EXOFS_ERR("Timout must be > 0");
    > + return -EINVAL;
    > + }
    > + opts->timeout = option * HZ;

    Is it intentional to be a different timeouton systems with different HX
    but the same mount option?

    > +static struct inode *exofs_alloc_inode(struct super_block *sb)
    > +{
    > + struct exofs_i_info *oi;
    > +
    > + oi = kmem_cache_alloc(exofs_inode_cachep, GFP_KERNEL);

    I'm curious if this should be GFP_NOFS or not?

    > + if (!oi)
    > + return NULL;
    > +
    > + oi->vfs_inode.i_version = 1;
    > + return &oi->vfs_inode;
    > +}

    > +static void exofs_put_super(struct super_block *sb)
    > +{
    > + int num_pend;
    > + struct exofs_sb_info *sbi = sb->s_fs_info;
    > +
    > + /* make sure there are no pending commands */
    > + for (num_pend = atomic_read(&sbi->s_curr_pending); num_pend > 0;
    > + num_pend = atomic_read(&sbi->s_curr_pending)) {

    This rises a question. Let's check exofs_new_inode() for example (it is
    a bad example, since inode can not be created when we already in the
    put_super() callback, but still there are others), it increments
    s_curr_pending way after inode was created, so is it possible that
    some in-flight callback is about to be executed and its subsequent
    s_curr_pending manipulation will not be detected by this loop?

    Should s_curr_pending increment be audited all over the code to be
    increased before the potential postponing command starts (which is not
    the case in exofs_new_inode() above)?

    > + wait_queue_head_t wq;
    > + init_waitqueue_head(&wq);
    > + wait_event_timeout(wq,
    > + (atomic_read(&sbi->s_curr_pending) == 0),
    > + msecs_to_jiffies(100));
    > + }
    > +
    > + osduld_put_device(sbi->s_dev);
    > + kfree(sb->s_fs_info);
    > + sb->s_fs_info = NULL;
    > +}

    --
    Evgeniy Polyakov


    \
     
     \ /
      Last update: 2009-02-15 18:27    [W:0.027 / U:0.928 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site