From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-15?Q?Luk=E1=A8_Czerner?= Subject: Re: [PATCH] ext4: fix reservation release on invalidatepage for delalloc fs Date: Fri, 19 Jun 2015 09:53:44 +0200 (CEST) Message-ID: References: <1433406301-29136-1-git-send-email-lczerner@redhat.com> <20150608151651.GD19168@thunk.org> Mime-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="8323328-1692286947-1434700429=:14650" Cc: linux-ext4@vger.kernel.org To: "Theodore Ts'o" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60810 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754059AbbFSHxv (ORCPT ); Fri, 19 Jun 2015 03:53:51 -0400 In-Reply-To: Sender: linux-ext4-owner@vger.kernel.org List-ID: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-1692286947-1434700429=:14650 Content-Type: TEXT/PLAIN; charset=ISO-8859-15 Content-Transfer-Encoding: 8BIT On Mon, 8 Jun 2015, LukᨠCzerner wrote: > Date: Mon, 8 Jun 2015 17:45:08 +0200 (CEST) > From: LukᨠCzerner > To: Theodore Ts'o > Cc: linux-ext4@vger.kernel.org > Subject: Re: [PATCH] ext4: fix reservation release on invalidatepage for > delalloc fs > > On Mon, 8 Jun 2015, Theodore Ts'o wrote: > > > Date: Mon, 8 Jun 2015 11:16:51 -0400 > > From: Theodore Ts'o > > To: Lukas Czerner > > Cc: linux-ext4@vger.kernel.org > > Subject: Re: [PATCH] ext4: fix reservation release on invalidatepage for > > delalloc fs > > > > On Thu, Jun 04, 2015 at 10:25:01AM +0200, Lukas Czerner wrote: > > > On delalloc enabled file system on invalidatepage operation > > > in ext4_da_page_release_reservation() we want to clear the delayed > > > buffer and remove the extent covering the delayed buffer from the extent > > > status tree. > > > > > > However currently there is a bug where on the systems with page size > > > > block size we will always remove extents from the start of the page > > > regardless where the actual delayed buffers are positioned in the page. > > > > Right, because we end up screwing up the accounting. > > > > > @@ -1363,14 +1363,23 @@ static void ext4_da_page_release_reservation(struct page *page, > > > > > > if ((offset <= curr_off) && (buffer_delay(bh))) { > > > to_release++; > > > + contiguous_blks++; > > > clear_buffer_delay(bh); > > > + } else if (contiguous_blks) { > > > + lblk = page->index << > > > + (PAGE_CACHE_SHIFT - inode->i_blkbits); > > > + lblk += (curr_off >> inode->i_blkbits) - > > > + contiguous_blks; > > > + ext4_es_remove_extent(inode, lblk, contiguous_blks); > > > + contiguous_blks = 0; > > > } > > > curr_off = next_off; > > > } while ((bh = bh->b_this_page) != head); > > > > Shouldn't we call ext4_es_remove_extent() on the portion of the page > > containing the delayed allocation region, before we clear > > contiguous_blks and resetting lblk? > > Hi Ted, > > right this is the point of the patch. > > > > > For example, suppose we had the 4k page with a 1k block size, where > > the first, second, and fourth blocks are delayed allocated. With this > > patch we will end up only clearing the extent status tree for the > > fourth block, but not the first and second. > > So when the first and second block are delayed, then the > > if ((offset <= curr_off) && (buffer_delay(bh))) > > will hit twice which means that we'll have contiguous_blks = 2 > > Now on the third block this condition will no longer be true > (because buffer_delay(bh) will be false) and so we will hit > > else if (contiguous_blks) { > > then lblk will be: start of the page + (curr_off - contiguous_blks). > curr_off at this point will point at third block (index 2) and > contiguous_blks is 2. Which means that lblk will point at the start > of the page - which is exactly right because the first delayed block > is at the start of the page. > > So ext4_es_remove_extent() will remove extent of two blocks starting > from the end of the page - which means it removes first and second > delayed block. > > Now when we check fourth block the > > if ((offset <= curr_off) && (buffer_delay(bh))) > > will hit again, leaving contiguous_blks with 1, then we leave the > while cycle and hit this: > > if (contiguous_blks) > > removing the extent starting at fourth block in the page removing > one block (the fourth block in the page). > > That's how I wrote the code, but maybe I am missing something ? I am > a bit tired today already so my explanation is not very good, sorry. > > Can you put your question in a form of a patch ? > > Thanks! > -Lukas Hi Ted, can you take a second look at the patch ? Thanks! -Lukas > > > > > - Ted > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > --8323328-1692286947-1434700429=:14650--