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

From: Andrea Arcangeli (andrea@suse.de)
Date: Sun Jan 06 2002 - 22:08:28 EST


On Sat, Jan 05, 2002 at 03:43:36AM -0800, Andrew Morton wrote:
> static int msync_interval(struct vm_area_struct * vma,
> unsigned long start, unsigned long end, int flags)
> {
> + int ret = 0;
> struct file * file = vma->vm_file;
> +
> if (file && (vma->vm_flags & VM_SHARED)) {
> - int error;
> - error = filemap_sync(vma, start, end-start, flags);
> + /* filemap_sync() cannot fail */

:) yes, I guess it cames from some old debugging code trying to catch
pte corruption, now dead in current kernels.

> + 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.

> 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).

> @@ -433,9 +439,9 @@ asmlinkage long sys_fdatasync(unsigned i
> struct file * file;
> struct dentry * dentry;
> struct inode * inode;
> - int err;
> + int ret, err;
>
> - err = -EBADF;
> + ret = -EBADF;
> file = fget(fd);
> if (!file)
> goto out;
> @@ -443,14 +449,18 @@ asmlinkage long sys_fdatasync(unsigned i
> dentry = file->f_dentry;
> inode = dentry->d_inode;
>
> - err = -EINVAL;
> + ret = -EINVAL;
> if (!file->f_op || !file->f_op->fsync)
> goto out_putf;
>
> down(&inode->i_sem);
> - filemap_fdatasync(inode->i_mapping);
> + ret = filemap_fdatasync(inode->i_mapping);
> err = file->f_op->fsync(file, dentry, 1);
> - filemap_fdatawait(inode->i_mapping);
> + if (err && !ret)
> + ret = err;
> + err = filemap_fdatawait(inode->i_mapping);
> + if (err && !ret)
> + ret = err;
> up(&inode->i_sem);
>
> out_putf:

you forgot return ret here.

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

> + status = filemap_fdatasync(inode->i_mapping);
> down(&inode->i_sem);
> - status = nfs_wb_all(inode);
> + status2 = nfs_wb_all(inode);
> + if (status2 && !status)
> + status = status2;
> up(&inode->i_sem);
> - filemap_fdatawait(inode->i_mapping);
> + status2 = filemap_fdatawait(inode->i_mapping);
> + if (status2 && !status)
> + status = status2;
> if (status < 0)
> return status;
>
> @@ -300,11 +308,17 @@ nfs_lock(struct file *filp, int cmd, str
> */
> out_ok:
> if ((IS_SETLK(cmd) || IS_SETLKW(cmd)) && fl->fl_type != F_UNLCK) {
> - filemap_fdatasync(inode->i_mapping);
> + status2 = filemap_fdatasync(inode->i_mapping);
> + if (status2 && !status)
> + status = status2;
> down(&inode->i_sem);
> - nfs_wb_all(inode); /* we may have slept */
> + status2 = nfs_wb_all(inode); /* we may have slept */
> + if (status2 && !status)
> + status2 = status;
> up(&inode->i_sem);
> - filemap_fdatawait(inode->i_mapping);
> + status2 = filemap_fdatawait(inode->i_mapping);
> + if (status2 && !status)
> + status = status2;
> nfs_zap_caches(inode);
> }
> return status;

all right, thanks.

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