xfs
[Top] [All Lists]

Re: [PATCH] incorrect error handling inside generic_file_direct_write

To: "Chen, Kenneth W" <kenneth.w.chen@xxxxxxxxx>
Subject: Re: [PATCH] incorrect error handling inside generic_file_direct_write
From: "'Christoph Hellwig'" <hch@xxxxxxxxxxxxx>
Date: Fri, 15 Dec 2006 10:43:41 +0000
Cc: "'Andrew Morton'" <akpm@xxxxxxxx>, Dmitriy Monakhov <dmonakhov@xxxxx>, "'Christoph Hellwig'" <hch@xxxxxxxxxxxxx>, Dmitriy Monakhov <dmonakhov@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, Linux Memory Management <linux-mm@xxxxxxxxx>, devel@xxxxxxxxxx, xfs@xxxxxxxxxxx
In-reply-to: <000001c71e60$7df9e010$e434030a@amr.corp.intel.com>
Mail-followup-to: 'Christoph Hellwig' <hch@xxxxxxxxxxxxx>, "Chen, Kenneth W" <kenneth.w.chen@xxxxxxxxx>, 'Andrew Morton' <akpm@xxxxxxxx>, Dmitriy Monakhov <dmonakhov@xxxxx>, Dmitriy Monakhov <dmonakhov@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, Linux Memory Management <linux-mm@xxxxxxxxx>, devel@xxxxxxxxxx, xfs@xxxxxxxxxxx
References: <20061212024027.6c2a79d3.akpm@osdl.org> <000001c71e60$7df9e010$e434030a@amr.corp.intel.com>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.2i
> +ssize_t
> +__generic_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
> +                             unsigned long nr_segs, loff_t pos)

I'd still call this generic_file_aio_write_nolock.

> +     loff_t          *ppos = &iocb->ki_pos;

I'd rather use iocb->ki_pos directly in the few places ppos is referenced
currently.

>       if (ret > 0 && ((file->f_flags & O_SYNC) || IS_SYNC(inode))) {
> -             ssize_t err;
> -
>               err = sync_page_range_nolock(inode, mapping, pos, ret);
>               if (err < 0)
>                       ret = err;
>       }

So we're doing the sync_page_range once in __generic_file_aio_write
with i_mutex held.


>       mutex_lock(&inode->i_mutex);
> -     ret = __generic_file_aio_write_nolock(iocb, iov, nr_segs,
> -                     &iocb->ki_pos);
> +     ret = __generic_file_aio_write(iocb, iov, nr_segs, pos);
>       mutex_unlock(&inode->i_mutex);
>  
>       if (ret > 0 && ((file->f_flags & O_SYNC) || IS_SYNC(inode))) {

And then another time after it's unlocked, this seems wrong.


<Prev in Thread] Current Thread [Next in Thread>