Re: ramdisk corruption problems - was: RE: pivot_root and initrd kern el panic woes

From: Andrew Morton (akpm@zip.com.au)
Date: Sun Jan 06 2002 - 22:49:05 EST


Andrea Arcangeli wrote:
>
> > + ret = filemap_sync(vma, start, end-start, flags);
> >
> > - if (!error && (flags & MS_SYNC)) {
> > + if (flags & (MS_SYNC|MS_ASYNC)) {
>
> ok, it cannot fail but I prefer you either avoid the 'ret =
> filemap_sync' and you make filemap_sync return void, or you also left
> '(!err' over here.

OK, let's leave the test in place - filemap_sync() has global scope.

> > struct inode * inode = file->f_dentry->d_inode;
> > +
> > down(&inode->i_sem);
> > - filemap_fdatasync(inode->i_mapping);
> > - if (file->f_op && file->f_op->fsync)
> > - error = file->f_op->fsync(file, file->f_dentry, 1);
> > - filemap_fdatawait(inode->i_mapping);
> > + ret = filemap_fdatasync(inode->i_mapping);
> > + if (flags & MS_SYNC) {
> > + int err;
> > +
> > + if (file->f_op && file->f_op->fsync) {
> > + err = file->f_op->fsync(file, file->f_dentry, 1);
> > + if (err && !ret)
> > + ret = err;
> > + }
> > + err = filemap_fdatawait(inode->i_mapping);
> > + if (err && !ret)
> > + ret = err;
> > + }
>
> sounds right (not something I'd love to do in 2.4 but it's
> strightforward enough so I'll take the risk).

It works OK, but in practice makes little difference compared to MS_ASYNC.
We run out of request slots very easily with this sort of thing.

Another option would be to make MS_ASYNC behave the same as MS_SYNC,
which is probably better than just ignoring it as we do at present.
I'll leave it as shown above unless there be objections.
 
>
> you forgot return ret here.

Whoop, thanks. It was returning `err'.
 
> > --- linux-2.4.18-pre1/fs/nfs/file.c Wed Dec 26 11:47:41 2001
> > +++ linux-akpm/fs/nfs/file.c Sat Jan 5 03:21:07 2002
> > @@ -244,6 +244,7 @@ nfs_lock(struct file *filp, int cmd, str
> > {
> > struct inode * inode = filp->f_dentry->d_inode;
> > int status = 0;
> > + int status2;
> >
> > dprintk("NFS: nfs_lock(f=%4x/%ld, t=%x, fl=%x, r=%Ld:%Ld)\n",
> > inode->i_dev, inode->i_ino,
> > @@ -278,11 +279,18 @@ nfs_lock(struct file *filp, int cmd, str
> > * Flush all pending writes before doing anything
> > * with locks..
> > */
> > - filemap_fdatasync(inode->i_mapping);
> > + /*
> > + * Shouldn't filemap_fdatasync/wait be inside i_sem?
> > + */
>
> I think it's not necessary, because the list browse it's serialized by
> the pagecache_lock and writepage can run outside the i_sem.

Yup. I've changed this part of the patch based on some discussion
with Trond.

I'll wait until Marcelo looks like he has his head above water and
then send out the final version of the ramdisk, truncate and
fsync/msync patches, cc'ed to yourself and lkml.

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



This archive was generated by hypermail 2b29 : Mon Jan 07 2002 - 21:00:33 EST