linux-kernel - Re: [PATCH RESEND 2/7] xfs: add support FALLOC_FL_COLLAPSE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKYAXd9Z3OY1Hw3sNMVQAwZH2SwZ9au+rB=RKupwYH2MR=2+aQ@mail.gmail.com>
Date:	Mon, 14 Oct 2013 12:30:09 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	viro@...iv.linux.org.uk, mtk.manpages@...il.com, tytso@....edu,
	adilger.kernel@...ger.ca, bpm@....com, elder@...nel.org,
	hch@...radead.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-ext4@...r.kernel.org,
	xfs@....sgi.com, a.sangwan@...sung.com,
	Namjae Jeon <namjae.jeon@...sung.com>
Subject: Re: [PATCH RESEND 2/7] xfs: add support FALLOC_FL_COLLAPSE_RANGE for fallocate

2013/10/11 Dave Chinner <david@...morbit.com>:
> On Thu, Oct 10, 2013 at 04:00:13PM +0900, Namjae Jeon wrote:
>> >
>> > /*
>> >  * Shift extent records to the left to cover a hole.
>> >  *
>> >  * The maximum number of extents to be shifted in a single operation
>> >  * is @count, and @current_ext keeps track of the current extent
>> >  * index we have shifted. If there is no hole to shift the extents
>> >  * into, then we abort immediately.
>> >  */
>> Thanks for your help. I will change this comment instead of original one.
>>
>> >> +int
>> >> +xfs_bmap_shift_extents(
>> >> +     struct xfs_trans        *tp,
>> >> +     struct xfs_inode        *ip,
>> >> +     int                     *done,
>> >> +     xfs_fileoff_t           start_fsb,
>> >> +     xfs_fileoff_t           shift,
>> >
>> > Shift means ...? Number of extents to shift, a length, a number of
>> > block, or something else?
>> Ah, yes, shift_len would be a more proper name
>
> I'm not sure that's a lot better. What are we shifting? We are
> shifting the offset of the blocks, right? And the unit is in fsb?
> So perhaps offset_shift_fsb, and add that to the description of the
> function above?
Okay, I will change it as your suggestion.

>
>> >> +             /*
>> >> +              * Before shifting extent into hole, make sure that the hole
>> >> +              * is large enough to accomodate the shift.
>> >> +              */
>> >> +             if (*current_ext) {
>> >> +                     state |= BMAP_LEFT_VALID;
>> >> +                     xfs_bmbt_get_all(xfs_iext_get_ext(ifp,
>> >> +                                             *current_ext - 1), &left);
>> >> +
>> >> +                     if (isnullstartblock(left.br_startblock))
>> >> +                             state |= BMAP_LEFT_DELAY;
>> >> +
>> >> +                     if (startoff < left.br_startoff + left.br_blockcount)
>> >> +                             error = XFS_ERROR(EFSCORRUPTED);
>> >
>> > Why is the filesystem corrupted if the shift we asked for is too
>> > large for the hole in the file? I haven't seen any checks before
>> > this that guarantee that the hole is big enough for the shift...
>>
>> we call xfs_free_file_space to free enough blocks for shifting.
>> If still the space is not big enough will it be considered as fs corrupted?
>> What error could we return in this case?
>
> Hole punching rounds inwards, and the amount of rounding is not
> necessarily the nearest filesystem block. Again it's the block size
> smaller than page size case that will trip you over here, as the
> rounding  when punching holes will be done to page size, not
> filesystem block size. Hence it's entirely possible that your
> calculated shift start and lengths don't match the size of the hole
> that was punched.
>
> That doesn't mean there was a corruption - just that the hole wasn't
> the size and shape that was expected....
Right. I will consider your points.

Thanks very much for your valuable comments.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/