On 21 May 2014 18:40, Vyacheslav Dubeyko <slava@xxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
