Re: oops in file_storage.c - Kernel

This is a discussion on Re: oops in file_storage.c - Kernel ; On Thu, 23 Oct 2008, Manish Lachwani wrote: > Hi Alan, > > > > > Hmm. On my system (2.6.27) the code has moved to fs/sync.c/do_fsync(), > > and it has changed a fair amount. Maybe file_storage.c should be ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: Re: oops in file_storage.c

  1. Re: oops in file_storage.c

    On Thu, 23 Oct 2008, Manish Lachwani wrote:

    > Hi Alan,
    >
    > >
    > > Hmm. On my system (2.6.27) the code has moved to fs/sync.c/do_fsync(),
    > > and it has changed a fair amount. Maybe file_storage.c should be
    > > changed to match.

    >
    > I have the latest git checkout of
    > http://www.kernel.org/pub/scm/linux/.../linux-2.6.git
    > and it seems that do_fsync() is still quite similar to fsync_sub().
    > Also, I checked older trees like 2.6.22 and do_fsync() is still in
    > fs/sync.c


    That just goes to show how old file_storage.c is.

    > > In fact, the easiest approach would be to EXPORT do_fsync() and then
    > > call it directly. I don't know whether people would like this, though.
    > >

    >
    > I could try this. But I don't see much difference b/w do_fsync() and
    > fsync_sub(). However in do_fsync(), filemap_fdatawrite() is called
    > before the mutex_lock() and filemap_fdatawait() is called after the
    > mutex_unlock().
    >
    > Here is do_fsync() -
    >
    > long do_fsync(struct file *file, int datasync)
    > {
    > int ret;
    > int err;
    > struct address_space *mapping = file->f_mapping;


    file_storage does

    inode = filp->f_path.dentry->d_inode;

    instead, and uses inode in place of mapping->host and inode->i_mapping
    in place of mapping. Should this be changed? I have no idea, beyond
    feeling that the core kernel code has got to be more up-to-date. Same
    goes for the ordering of the locks and the filemap_* calls.

    > if (!file->f_op || !file->f_op->fsync) {
    > /* Why? We can still call filemap_fdatawrite */
    > ret = -EINVAL;
    > goto out;
    > }
    >
    > ret = filemap_fdatawrite(mapping);
    >
    > /*
    > * We need to protect against concurrent writers, which could cause
    > * livelocks in fsync_buffers_list().
    > */
    > mutex_lock(&mapping->host->i_mutex);
    > err = file->f_op->fsync(file, file->f_path.dentry, datasync);
    > if (!ret)
    > ret = err;
    > mutex_unlock(&mapping->host->i_mutex);
    > err = filemap_fdatawait(mapping);
    > if (!ret)
    > ret = err;
    > out:
    > return ret;
    > }


    If the EXPORT route is acceptable, I'd prefer to use it.

    Alan Stern

    --
    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/

  2. Re: oops in file_storage.c

    Hi Alan,

    Yes, I am trying out the changes below. Will let you know how it goes.
    If it works well, I will send an updated patch.

    diff --git a/drivers/usb/gadget/file_storage.c
    b/drivers/usb/gadget/file_storage.c
    index 3e807f0..844f992 100644
    --- a/drivers/usb/gadget/file_storage.c
    +++ b/drivers/usb/gadget/file_storage.c
    @@ -1927,26 +1927,11 @@ static int do_write(struct fsg_dev *fsg)
    static int fsync_sub(struct lun *curlun)
    {
    struct file *filp = curlun->filp;
    - struct inode *inode;
    - int rc, err;

    if (curlun->ro || !filp)
    return 0;
    - if (!filp->f_op || !filp->f_op->fsync)
    - return -EINVAL;

    - inode = filp->f_path.dentry->d_inode;
    - mutex_lock(&inode->i_mutex);
    - rc = filemap_fdatawrite(inode->i_mapping);
    - err = filp->f_op->fsync(filp, filp->f_path.dentry, 1);
    - if (!rc)
    - rc = err;
    - err = filemap_fdatawait(inode->i_mapping);
    - if (!rc)
    - rc = err;
    - mutex_unlock(&inode->i_mutex);
    - VLDBG(curlun, "fdatasync -> %d\n", rc);
    - return rc;
    + return do_fsync(filp, 0);
    }

    static void fsync_all(struct fsg_dev *fsg)
    diff --git a/fs/sync.c b/fs/sync.c
    index e700eb1..53ff1f3 100644
    --- a/fs/sync.c
    +++ b/fs/sync.c
    @@ -111,6 +111,7 @@ long do_fsync(struct file *file, int datasync)
    out:
    return ret;
    }
    +EXPORT_SYMBOL(do_fsync);

    static long __do_fsync(unsigned int fd, int datasync)
    {






    On Thu, Oct 23, 2008 at 9:46 AM, Alan Stern wrote:
    > On Thu, 23 Oct 2008, Manish Lachwani wrote:
    >
    >> Hi Alan,
    >>
    >> >
    >> > Hmm. On my system (2.6.27) the code has moved to fs/sync.c/do_fsync(),
    >> > and it has changed a fair amount. Maybe file_storage.c should be
    >> > changed to match.

    >>
    >> I have the latest git checkout of
    >> http://www.kernel.org/pub/scm/linux/.../linux-2.6.git
    >> and it seems that do_fsync() is still quite similar to fsync_sub().
    >> Also, I checked older trees like 2.6.22 and do_fsync() is still in
    >> fs/sync.c

    >
    > That just goes to show how old file_storage.c is.
    >
    >> > In fact, the easiest approach would be to EXPORT do_fsync() and then
    >> > call it directly. I don't know whether people would like this, though.
    >> >

    >>
    >> I could try this. But I don't see much difference b/w do_fsync() and
    >> fsync_sub(). However in do_fsync(), filemap_fdatawrite() is called
    >> before the mutex_lock() and filemap_fdatawait() is called after the
    >> mutex_unlock().
    >>
    >> Here is do_fsync() -
    >>
    >> long do_fsync(struct file *file, int datasync)
    >> {
    >> int ret;
    >> int err;
    >> struct address_space *mapping = file->f_mapping;

    >
    > file_storage does
    >
    > inode = filp->f_path.dentry->d_inode;
    >
    > instead, and uses inode in place of mapping->host and inode->i_mapping
    > in place of mapping. Should this be changed? I have no idea, beyond
    > feeling that the core kernel code has got to be more up-to-date. Same
    > goes for the ordering of the locks and the filemap_* calls.
    >
    >> if (!file->f_op || !file->f_op->fsync) {
    >> /* Why? We can still call filemap_fdatawrite */
    >> ret = -EINVAL;
    >> goto out;
    >> }
    >>
    >> ret = filemap_fdatawrite(mapping);
    >>
    >> /*
    >> * We need to protect against concurrent writers, which could cause
    >> * livelocks in fsync_buffers_list().
    >> */
    >> mutex_lock(&mapping->host->i_mutex);
    >> err = file->f_op->fsync(file, file->f_path.dentry, datasync);
    >> if (!ret)
    >> ret = err;
    >> mutex_unlock(&mapping->host->i_mutex);
    >> err = filemap_fdatawait(mapping);
    >> if (!ret)
    >> ret = err;
    >> out:
    >> return ret;
    >> }

    >
    > If the EXPORT route is acceptable, I'd prefer to use it.
    >
    > Alan Stern
    >
    >

    --
    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/

  3. Re: oops in file_storage.c

    On Thu, 23 Oct 2008, Manish Lachwani wrote:

    > Hi Alan,
    >
    > Yes, I am trying out the changes below. Will let you know how it goes.
    > If it works well, I will send an updated patch.
    >
    > diff --git a/drivers/usb/gadget/file_storage.c
    > b/drivers/usb/gadget/file_storage.c
    > index 3e807f0..844f992 100644
    > --- a/drivers/usb/gadget/file_storage.c
    > +++ b/drivers/usb/gadget/file_storage.c
    > @@ -1927,26 +1927,11 @@ static int do_write(struct fsg_dev *fsg)
    > static int fsync_sub(struct lun *curlun)
    > {
    > struct file *filp = curlun->filp;
    > - struct inode *inode;
    > - int rc, err;
    >
    > if (curlun->ro || !filp)
    > return 0;
    > - if (!filp->f_op || !filp->f_op->fsync)
    > - return -EINVAL;
    >
    > - inode = filp->f_path.dentry->d_inode;
    > - mutex_lock(&inode->i_mutex);
    > - rc = filemap_fdatawrite(inode->i_mapping);
    > - err = filp->f_op->fsync(filp, filp->f_path.dentry, 1);
    > - if (!rc)
    > - rc = err;
    > - err = filemap_fdatawait(inode->i_mapping);
    > - if (!rc)
    > - rc = err;
    > - mutex_unlock(&inode->i_mutex);
    > - VLDBG(curlun, "fdatasync -> %d\n", rc);
    > - return rc;
    > + return do_fsync(filp, 0);


    The fsync call above uses 1, not 0.

    > }
    >
    > static void fsync_all(struct fsg_dev *fsg)
    > diff --git a/fs/sync.c b/fs/sync.c
    > index e700eb1..53ff1f3 100644
    > --- a/fs/sync.c
    > +++ b/fs/sync.c
    > @@ -111,6 +111,7 @@ long do_fsync(struct file *file, int datasync)
    > out:
    > return ret;
    > }
    > +EXPORT_SYMBOL(do_fsync);
    >
    > static long __do_fsync(unsigned int fd, int datasync)
    > {


    Apart from that one mistake, this is fine with me. But you should
    check with the core kernel developers; there may an objection to
    exporting do_fsync().

    Alan Stern

    --
    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/

+ Reply to Thread