From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Antonov Subject: Re: [PATCH] hfsplus: fix "unused node is not erased" error Date: Wed, 21 May 2014 20:15:17 +0200 Message-ID: References: <1400607863-14633-1-git-send-email-saproj@gmail.com> <1400690445.15260.14.camel@slavad-CELSIUS-H720> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: "linux-fsdevel@vger.kernel.org" , Anton Altaparmakov , Al Viro , Christoph Hellwig , Hin-Tak Leung , Kyle Laracey , Andrew Morton To: Vyacheslav Dubeyko Return-path: Received: from mail-oa0-f54.google.com ([209.85.219.54]:41781 "EHLO mail-oa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751137AbaEUSPR (ORCPT ); Wed, 21 May 2014 14:15:17 -0400 Received: by mail-oa0-f54.google.com with SMTP id j17so2735196oag.27 for ; Wed, 21 May 2014 11:15:17 -0700 (PDT) In-Reply-To: <1400690445.15260.14.camel@slavad-CELSIUS-H720> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 21 May 2014 18:40, Vyacheslav Dubeyko wrote: > On Tue, 2014-05-20 at 19:44 +0200, Sergei Antonov wrote: > > [snip] >> >> -int hfsplus_file_extend(struct inode *inode) >> +int hfsplus_file_extend(struct inode *inode, bool zeroout) >> { >> struct super_block *sb = inode->i_sb; >> struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb); >> @@ -463,6 +463,12 @@ int hfsplus_file_extend(struct inode *inode) >> } >> } >> >> + if (zeroout) { >> + res = sb_issue_zeroout(sb, start, len, GFP_NOFS); > > As I can see, sb_issue_zeroout() initiate request for write. But > previously the hfsplus_file_extend() operated by page cache only during > file extending. From one point of view, we can fail during operation of > file extending but, anyway, we will zero out blocks by means of writing. Which is not bad. Those blocks are free space. > From another point of view, prepared pages are returned as tree's nodes > for filling by some data and, finally, it will be written on volume as a > result of node creation. A result of node creation is only 1 node, but catalog file is expanded in clumps. Normally a clump is at least several megabytes. So the task is to zero these megabytes on disk before (or immediately after) the new extent is added to the catalog. > So, I think that it makes sense to zero out namely prepared pages but > not to initiate request for write via sb_issue_zeroout(). You mean mapping pages, do memset(,0,) and flushing them? Slower, memory consuming, complicated. >> + if (res) >> + goto out; >> + } >> + >> hfs_dbg(EXTENT, "extend %lu: %u,%u\n", inode->i_ino, start, len); >> >> if (hip->alloc_blocks <= hip->first_blocks) { >> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h >> index 83dc292..3c872b2 100644 >> --- a/fs/hfsplus/hfsplus_fs.h >> +++ b/fs/hfsplus/hfsplus_fs.h >> @@ -417,6 +417,7 @@ void hfs_bnode_free(struct hfs_bnode *); >> struct hfs_bnode *hfs_bnode_create(struct hfs_btree *, u32); >> void hfs_bnode_get(struct hfs_bnode *); >> void hfs_bnode_put(struct hfs_bnode *); >> +bool hfs_bnode_need_zeroout(struct hfs_btree *); >> >> /* brec.c */ >> u16 hfs_brec_lenoff(struct hfs_bnode *, u16, u16 *); >> @@ -463,7 +464,7 @@ int hfsplus_ext_write_extent(struct inode *); >> int hfsplus_get_block(struct inode *, sector_t, struct buffer_head *, int); >> int hfsplus_free_fork(struct super_block *, u32, >> struct hfsplus_fork_raw *, int); >> -int hfsplus_file_extend(struct inode *); >> +int hfsplus_file_extend(struct inode *, bool zeroout); > > I think that it doesn't make sense to keep name of second argument here. Yes, for consistency with other prototypes.