On 23 May 2014 08:21, Vyacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
> On Thu, 2014-05-22 at 15:18 +0200, Sergei Antonov wrote:
>> On 22 May 2014 15:07, Vyacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
>> > On Wed, 2014-05-21 at 20:15 +0200, Sergei Antonov wrote:
>> >> 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.
>> >>
>> >
>> > For me personally, proper place for sb_issue_zeroout() can be in
>> > hfs_bmap_alloc() method
>> > (http://lxr.free-electrons.com/source/fs/hfsplus/btree.c#L364):
>> >
>> >
>> > while (!tree->free_nodes) {
>> > struct inode *inode = tree->inode;
>> > struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
>> > u32 count;
>> > int res;
>> >
>> > res = hfsplus_file_extend(inode);
>> > if (res)
>> > return ERR_PTR(res);
>> >
>> > /* here can be added sb_issue_zeroout() call */
>> >
>> > hip->phys_size = inode->i_size =
>> > (loff_t)hip->alloc_blocks <<
>> > HFSPLUS_SB(tree->sb)->alloc_blksz_shift;
>> > hip->fs_blocks =
>> > hip->alloc_blocks << HFSPLUS_SB(tree->sb)->fs_shift;
>> > inode_set_bytes(inode, inode->i_size);
>> > count = inode->i_size >> tree->node_size_shift;
>> > tree->free_nodes = count - tree->node_count;
>> > tree->node_count = count;
>> > }
>> >
>> > First of all, here we know that trying to extend file was successful.
>> > And, secondly, hfs_bmap_alloc() method is dedicated b-tree case only.
>> > I think that modification of hfsplus_file_extend() is not very good
>> > idea. The hfs_bmap_alloc() method is more clear solution, from my
>> > viewpoint.
>>
>> hfs_bmap_alloc() does not know about volume blocks. It is on a higher
>> level of abstraction. Try, as an experiment, to write a call to
>> sb_issue_zeroout() passing correct arguments from hfs_bmap_alloc().
>>
>
> The hfs_bmap_alloc() has pointer on hfsplus_inode_info structure of
> btree's inode. The hfsplus_inode_info structure contains
> hip->first_extents, hip->first_blocks, hip->cached_extents,
> hip->cached_blocks and so on fields. Finally, hfsplus_file_extend()
> method stores allocated extent in hip->first_extents or in
> hip->cached_extents.
Yo-ho! What a ride!
> So, I don't see anything impossible in calling
> sb_issue_zeroout() with correct arguments from hfs_bmap_alloc().
And you seem to like riding.
> Maybe
> only to call sb_issue_zeroout() from hfsplus_file_extend() is more easy
> way. But I think that placement this logic in hfs_bmap_alloc() is more
> correct way, from architecture point of view.
It is nasty from an architectural point of view for reasons provided
earlier. hfsplus_file_extend() is volume blocks level and that's what
one needs for sb_issue_*.
--
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
