From: Cyril Hrubis <chrubis@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse()
Date: Thu, 9 Feb 2017 10:31:52 +0100 [thread overview]
Message-ID: <20170209093152.GA12673@rei.lan> (raw)
In-Reply-To: <589C2EA5.6040707@cn.fujitsu.com>
Hi!
> >>> memset(bufptr, 0, writesize);
> >>> lseek(fd, offset, SEEK_SET);
> >>> - for (i = offset; i < filesize;) {
> >>> + for (i = 0; i < filesize;) {
> >>> if ((w = write(fd, bufptr, writesize)) != writesize) {
> >>> tst_resm(TBROK | TERRNO, "write() returned %d", w);
> >>> close(fd);
> >>
> >> Hmm, it looks to me like the code actually does what it should have.
> >> Since we pass a filesize and offset so the test should actually write
> >> filesize - offset bytes. As far as I can tell the bug here is that the
> >> test is not checking that filesize > offset before it starts.
> >
> > Sorry, but shouldn't we write the whole "writesize" from the "offset"?
> > ADSP080 ADSP081 ADSP082 ADSP083 did reproduce the BUG via above modification...
>
> I think we pass an "offset" is to make sure writing from the file offset,
> but the actual write size should be just the "writesize" argument.
>
> As the second case of kernel commit 9ecd10b says:
> 2. Direct writes starting from or beyong i_size (not inside i_size)
> also could trigger block allocation and expose stale data. For
> example, consider a sparse file with i_size of 2k, and a write to
> offset 2k or 3k into the file, with a filesystem block size of 4k.
> (Thanks to Jeff Moyer for pointing this case out in his review.)
Ah, looking at the git log, the offset was just added recently, that's
why the code is confusing, previously the filesize was size of the
file...
So what about we rename the writesize to blocksize and filesize to
writesize so that it's clear what the function dio_sparse does.
int dio_sparse(char *filename, int align, int blocksize, int writesize, int offset)
Also shouldn't we truncate the file to offset + filesize rather than
just filesize? And then pass the size to the children as offset +
filesize as well?
--
Cyril Hrubis
chrubis@suse.cz
next prev parent reply other threads:[~2017-02-09 9:31 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-19 9:32 [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Guangwen Feng
2017-01-19 9:32 ` [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() Guangwen Feng
2017-01-24 13:43 ` Cyril Hrubis
2017-01-25 4:02 ` Guangwen Feng
2017-02-09 7:23 ` [LTP] [PATCH v2] " Guangwen Feng
2017-02-09 9:35 ` Cyril Hrubis
2017-02-14 3:20 ` Guangwen Feng
2017-03-21 8:08 ` [LTP] [PATCH] ltp-aiodio: Create the file before fork Guangwen Feng
2017-03-22 15:33 ` Cyril Hrubis
2017-01-24 13:40 ` [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Cyril Hrubis
2017-01-25 4:33 ` Guangwen Feng
2017-02-09 8:56 ` Guangwen Feng
2017-02-09 9:31 ` Cyril Hrubis [this message]
2017-02-14 8:53 ` Guangwen Feng
2017-02-16 13:12 ` Cyril Hrubis
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=20170209093152.GA12673@rei.lan \
--to=chrubis@suse.cz \
--cc=ltp@lists.linux.it \
/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