From: Roman Penyaev Subject: Re: [PATCH v2 1/2] ext4: Include forgotten start block on fallocate insert range Date: Mon, 9 Jan 2017 13:36:34 +0100 Message-ID: References: <20170106202601.32722-1-roman.penyaev@profitbricks.com> <20170106202601.32722-2-roman.penyaev@profitbricks.com> <20170107212210.2repgu7cm6mtimek@thunk.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 To: "Theodore Ts'o" , Roman Pen , Namjae Jeon , Andreas Dilger , linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org Return-path: In-Reply-To: <20170107212210.2repgu7cm6mtimek@thunk.org> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-ext4.vger.kernel.org On Sat, Jan 7, 2017 at 10:22 PM, Theodore Ts'o wrote: > On Fri, Jan 06, 2017 at 09:26:00PM +0100, Roman Pen wrote: >> While doing 'insert range' start block should be also shifted right. >> The bug can be easily reproduced by the following test: >> >> ptr = malloc(4096); >> assert(ptr); >> >> fd = open("./ext4.file", O_CREAT | O_TRUNC | O_RDWR, 0600); >> assert(fd >= 0); >> >> rc = fallocate(fd, 0, 0, 8192); >> assert(rc == 0); >> for (i = 0; i < 2048; i++) >> *((unsigned short *)ptr + i) = 0xbeef; >> rc = pwrite(fd, ptr, 4096, 0); >> assert(rc == 4096); >> rc = pwrite(fd, ptr, 4096, 4096); >> assert(rc == 4096); >> >> for (block = 2; block < 1000; block++) { >> rc = fallocate(fd, FALLOC_FL_INSERT_RANGE, 4096, 4096); >> assert(rc == 0); >> >> for (i = 0; i < 2048; i++) >> *((unsigned short *)ptr + i) = block; >> >> rc = pwrite(fd, ptr, 4096, 4096); >> assert(rc == 4096); >> } >> >> Because start block is not included in the range the hole appears at >> the wrong offset (just after the desired offset) and the following >> pwrite() overwrites already existent block, keeping hole untouched. >> >> Simple way to verify wrong behaviour is to check zeroed blocks after >> the test: >> >> $ hexdump ./ext4.file | grep '0000 0000' >> >> The root cause of the bug is a wrong range (start, stop], where start >> should be inclusive, i.e. [start, stop]. >> >> This patch fixes the problem by including start into the range. But >> not to break left shift (range collapse) stop points to the beginning >> of the a block, not to the end. >> >> The other not obvious change is an iterator check on validness in a >> main loop. Because iterator is unsigned the following corner case >> should be considered with care: insert a block at 0 offset, when stop >> variables overflows and never becomes less than start, which is 0. >> To handle this special case iterator is set to NULL to indicate that >> end of the loop is reached. >> >> Signed-off-by: Roman Pen > > Thanks, applied. > Could you please provide with the SHA1 of the patch in your branch? I want to make an exact reference in a new test of the xfstest which covers that bug. -- Roman