From: Jan Kara <jack@suse.cz>
To: Brian Foster <bfoster@redhat.com>
Cc: Jan Kara <jack@suse.cz>,
"Darrick J . Wong" <darrick.wong@oracle.com>,
linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/2] xfs: Move handling of missing page into one place in xfs_find_get_desired_pgoff()
Date: Mon, 15 May 2017 16:44:21 +0200 [thread overview]
Message-ID: <20170515144421.GJ16182@quack2.suse.cz> (raw)
In-Reply-To: <20170515132834.GB27809@bfoster.bfoster>
On Mon 15-05-17 09:28:37, Brian Foster wrote:
> On Thu, May 11, 2017 at 06:50:23PM +0200, Jan Kara wrote:
> > Currently several places in xfs_find_get_desired_pgoff() handle the case
> > of a missing page. Make them all handled in one place after the loop has
> > terminated.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/xfs/xfs_file.c | 24 ++++++++----------------
> > 1 file changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index df51c025adfe..719923b99ba1 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
> > break;
> >
> > ASSERT(type == HOLE_OFF);
> > - if (lastoff == startoff || lastoff < endoff) {
> > - found = true;
> > - *offset = lastoff;
> > - }
> > break;
> > }
> >
> > @@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
> > * At least we found one page. If the current offset is smaller
> > * than the first page offset, a hole was found.
> > */
> > - if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
> > - found = true;
> > - *offset = lastoff;
> > + if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
> > break;
> > - }
> >
> > for (i = 0; i < nr_pages; i++) {
> > struct page *page = pvec.pages[i];
>
> FWIW, it looks like there's still a bug here that the first patch
> doesn't address:
>
> # xfs_io -fc "truncate 0" -c "falloc 0 32k" -c "pwrite 0 4k" -c "pwrite 32k 4k" -c "seek -h 0" /mnt/file
> wrote 4096/4096 bytes at offset 0
> 4 KiB, 1 ops; 0.0000 sec (126.008 MiB/sec and 32258.0645 ops/sec)
> wrote 4096/4096 bytes at offset 32768
> 4 KiB, 1 ops; 0.0000 sec (102.796 MiB/sec and 26315.7895 ops/sec)
> Whence Result
> HOLE 36864
> # xfs_io -c fsync -c "seek -h 0" /mnt/file
> Whence Result
> HOLE 4096
>
> I initially thought this patch might fix it, but on further digging it
> doesn't appear to. What looks like is going on here is that the
> 'page->index > end' check doesn't actually catch this case of the page
> being just beyond the current unwritten extent (perhaps the check should
> be 'page->index >= end'), so lastoff is set beyond end and we skip the
> hole.
Right, that looks like another bug in the implementation. Will you send a
fix or should I?
Honza
>
> Brian
>
> > @@ -1150,21 +1143,20 @@ xfs_find_get_desired_pgoff(
> >
> > /*
> > * The number of returned pages less than our desired, search
> > - * done. In this case, nothing was found for searching data,
> > - * but we found a hole behind the last offset.
> > + * done.
> > */
> > - if (nr_pages < want) {
> > - if (type == HOLE_OFF) {
> > - *offset = lastoff;
> > - found = true;
> > - }
> > + if (nr_pages < want)
> > break;
> > - }
> >
> > index = pvec.pages[i - 1]->index + 1;
> > pagevec_release(&pvec);
> > } while (index <= end);
> >
> > + /* No page at lastoff and we are not done - we found a hole. */
> > + if (type == HOLE_OFF && lastoff < endoff) {
> > + *offset = lastoff;
> > + found = true;
> > + }
> > out:
> > pagevec_release(&pvec);
> > return found;
> > --
> > 2.12.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2017-05-15 14:44 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-11 16:50 [PATCH 1/2] xfs: Fix missed holes in SEEK_HOLE implementation Jan Kara
2017-05-11 16:50 ` [PATCH 2/2] xfs: Move handling of missing page into one place in xfs_find_get_desired_pgoff() Jan Kara
2017-05-12 16:23 ` Darrick J. Wong
2017-05-17 12:15 ` Jan Kara
2017-05-15 13:28 ` Brian Foster
2017-05-15 14:44 ` Jan Kara [this message]
2017-05-15 14:55 ` Brian Foster
2017-05-16 11:42 ` Jan Kara
2017-05-15 16:09 ` Jan Kara
2017-05-12 16:23 ` [PATCH 1/2] xfs: Fix missed holes in SEEK_HOLE implementation Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170515144421.GJ16182@quack2.suse.cz \
--to=jack@suse.cz \
--cc=bfoster@redhat.com \
--cc=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).