* [PATCH] fs: make sure to invalidate pages if we fall back on buffered reads
@ 2010-06-08 14:24 Josef Bacik
2010-06-11 19:22 ` Jeff Moyer
2010-06-14 8:17 ` Christoph Hellwig
0 siblings, 2 replies; 4+ messages in thread
From: Josef Bacik @ 2010-06-08 14:24 UTC (permalink / raw)
To: linux-btrfs, linux-fsdevel, linux-kernel
Since BTRFS can fallback on buffered reads after having done some direct reads,
we need to make sure to invalidate any pages that we may have read by doing
buffered IO. This shouldn't have shown up as a visible user problem, it's just
for correctness sake. Thanks,
Signed-off-by: Josef Bacik <josef@redhat.com>
---
mm/filemap.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index 829ac9c..ca5aba9 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1266,6 +1266,7 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
unsigned long seg = 0;
size_t count;
loff_t *ppos = &iocb->ki_pos;
+ bool invalidate = false;
count = 0;
retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
@@ -1291,7 +1292,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
iov, pos, nr_segs);
}
if (retval > 0) {
- *ppos = pos + retval;
+ pos += retval;
+ *ppos = pos;
count -= retval;
}
@@ -1307,6 +1309,7 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
file_accessed(filp);
goto out;
}
+ invalidate = true;
}
}
@@ -1343,6 +1346,10 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
if (desc.count > 0)
break;
}
+ if (invalidate && retval > 0)
+ invalidate_mapping_pages(filp->f_mapping,
+ pos >> PAGE_CACHE_SHIFT,
+ (*ppos - 1) >> PAGE_CACHE_SHIFT);
out:
return retval;
}
--
1.6.6.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] fs: make sure to invalidate pages if we fall back on buffered reads
2010-06-08 14:24 [PATCH] fs: make sure to invalidate pages if we fall back on buffered reads Josef Bacik
@ 2010-06-11 19:22 ` Jeff Moyer
2010-06-14 8:17 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: Jeff Moyer @ 2010-06-11 19:22 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, linux-fsdevel, linux-kernel
Josef Bacik <josef@redhat.com> writes:
> Since BTRFS can fallback on buffered reads after having done some direct reads,
> we need to make sure to invalidate any pages that we may have read by doing
> buffered IO. This shouldn't have shown up as a visible user problem, it's just
> for correctness sake. Thanks,
This looks right to me. You definitely don't want to fill up the page
cache from direct I/O.
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
> mm/filemap.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 829ac9c..ca5aba9 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1266,6 +1266,7 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> unsigned long seg = 0;
> size_t count;
> loff_t *ppos = &iocb->ki_pos;
> + bool invalidate = false;
>
> count = 0;
> retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
> @@ -1291,7 +1292,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> iov, pos, nr_segs);
> }
> if (retval > 0) {
> - *ppos = pos + retval;
> + pos += retval;
> + *ppos = pos;
> count -= retval;
> }
>
> @@ -1307,6 +1309,7 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> file_accessed(filp);
> goto out;
> }
> + invalidate = true;
> }
> }
>
> @@ -1343,6 +1346,10 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> if (desc.count > 0)
> break;
> }
> + if (invalidate && retval > 0)
> + invalidate_mapping_pages(filp->f_mapping,
> + pos >> PAGE_CACHE_SHIFT,
> + (*ppos - 1) >> PAGE_CACHE_SHIFT);
> out:
> return retval;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fs: make sure to invalidate pages if we fall back on buffered reads
2010-06-08 14:24 [PATCH] fs: make sure to invalidate pages if we fall back on buffered reads Josef Bacik
2010-06-11 19:22 ` Jeff Moyer
@ 2010-06-14 8:17 ` Christoph Hellwig
2010-06-14 13:13 ` Josef Bacik
1 sibling, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2010-06-14 8:17 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, linux-fsdevel, linux-kernel
On Tue, Jun 08, 2010 at 10:24:04AM -0400, Josef Bacik wrote:
> Since BTRFS can fallback on buffered reads after having done some direct reads,
> we need to make sure to invalidate any pages that we may have read by doing
> buffered IO. This shouldn't have shown up as a visible user problem, it's just
> for correctness sake. Thanks,
Everything else in direct I/O land uses invalidate_inode_pages2(_range),
why not this one?
> loff_t *ppos = &iocb->ki_pos;
> + bool invalidate = false;
>
> count = 0;
> retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
> @@ -1291,7 +1292,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> iov, pos, nr_segs);
> }
> if (retval > 0) {
> - *ppos = pos + retval;
> + pos += retval;
> + *ppos = pos;
> count -= retval;
> }
>
> @@ -1307,6 +1309,7 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> file_accessed(filp);
> goto out;
> }
> + invalidate = true;
> }
> }
>
> @@ -1343,6 +1346,10 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> if (desc.count > 0)
> break;
> }
> + if (invalidate && retval > 0)
> + invalidate_mapping_pages(filp->f_mapping,
> + pos >> PAGE_CACHE_SHIFT,
> + (*ppos - 1) >> PAGE_CACHE_SHIFT);
A little comment here would be surely useful. Telling that we want to
get rid of the pages again if we were falling through from an attempted
direct I/O read.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fs: make sure to invalidate pages if we fall back on buffered reads
2010-06-14 8:17 ` Christoph Hellwig
@ 2010-06-14 13:13 ` Josef Bacik
0 siblings, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2010-06-14 13:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Josef Bacik, linux-btrfs, linux-fsdevel, linux-kernel
On Mon, Jun 14, 2010 at 04:17:36AM -0400, Christoph Hellwig wrote:
> On Tue, Jun 08, 2010 at 10:24:04AM -0400, Josef Bacik wrote:
> > Since BTRFS can fallback on buffered reads after having done some direct reads,
> > we need to make sure to invalidate any pages that we may have read by doing
> > buffered IO. This shouldn't have shown up as a visible user problem, it's just
> > for correctness sake. Thanks,
>
> Everything else in direct I/O land uses invalidate_inode_pages2(_range),
> why not this one?
>
In __generic_file_aio_write if we fall back on buffered writes we call
invalidate_mapping_pages to invalidate the buffered range, so I just did that
here, is that acceptable?
> > loff_t *ppos = &iocb->ki_pos;
> > + bool invalidate = false;
> >
> > count = 0;
> > retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
> > @@ -1291,7 +1292,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> > iov, pos, nr_segs);
> > }
> > if (retval > 0) {
> > - *ppos = pos + retval;
> > + pos += retval;
> > + *ppos = pos;
> > count -= retval;
> > }
> >
> > @@ -1307,6 +1309,7 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> > file_accessed(filp);
> > goto out;
> > }
> > + invalidate = true;
> > }
> > }
> >
> > @@ -1343,6 +1346,10 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> > if (desc.count > 0)
> > break;
> > }
> > + if (invalidate && retval > 0)
> > + invalidate_mapping_pages(filp->f_mapping,
> > + pos >> PAGE_CACHE_SHIFT,
> > + (*ppos - 1) >> PAGE_CACHE_SHIFT);
>
> A little comment here would be surely useful. Telling that we want to
> get rid of the pages again if we were falling through from an attempted
> direct I/O read.
>
Will do. I'll send an updated patch when you answer my above question. Thanks
for the review.
Josef
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-06-14 13:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-08 14:24 [PATCH] fs: make sure to invalidate pages if we fall back on buffered reads Josef Bacik
2010-06-11 19:22 ` Jeff Moyer
2010-06-14 8:17 ` Christoph Hellwig
2010-06-14 13:13 ` Josef Bacik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).