xfs
[Top] [All Lists]

Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support
From: Zhi Yong Wu <zwu.kernel@xxxxxxxxx>
Date: Thu, 28 Nov 2013 09:48:29 +0800
Cc: xfstests <xfs@xxxxxxxxxxx>, Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=GAgHS/Rgk59vYZsBjdrSLG6vpHIdSklQ9s0x7q4B4ZM=; b=mzcOb6YzXu9WgP+Z7qK8a/dpI00+rTJ24MPY5uxs7v60Y2FrnxlM3tplxgGGY19UBb lv8REMqa5J2wuE0r31z1fj4hji3eyR6EZ0hME6ZTDb8fs3vDEOjKS8Zdd4D7rutcb7JX zQJwLQ8CejMo6A4wYgB5lStLPa14mLKNiRl2eFo2EVjHme0jDP7umlE3yDgpCfv/lhDR /aE8HcqXzaaELqamyyzCSXyy4EfRl185yW0DZWlL98J482NEGdV7xF5OUfew3VMWuMLv 7RQIegnS7yBPqR8dmGJBQT63wKitXTsqw/Md9XAwytweGCe+00x9wRbGj8EY+YVWyrMf IqmQ==
In-reply-to: <20131125134821.GB30189@xxxxxxxxxxxxx>
References: <1385379154-3802-1-git-send-email-zwu.kernel@xxxxxxxxx> <1385379154-3802-3-git-send-email-zwu.kernel@xxxxxxxxx> <20131125134821.GB30189@xxxxxxxxxxxxx>
OK, will fix or cleanup all the comments, thanks.

On Mon, Nov 25, 2013 at 9:48 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> On Mon, Nov 25, 2013 at 07:32:32PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
>>
>> The function is used to create one O_TMPFILE file.
>>
>> http://oss.sgi.com/archives/xfs/2013-08/msg00339.html
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
>> ---
>>  fs/xfs/xfs_inode.c       |  129 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/xfs/xfs_inode.h       |    2 +
>>  fs/xfs/xfs_shared.h      |    4 +-
>>  fs/xfs/xfs_trans_resv.c  |   35 ++++++++++++
>>  fs/xfs/xfs_trans_resv.h  |    2 +
>>  fs/xfs/xfs_trans_space.h |    2 +
>>  6 files changed, 173 insertions(+), 1 deletions(-)
>
>>  int
>> +xfs_create_tmpfile(
>> +     xfs_mount_t             *mp,
>> +     umode_t                 mode,
>> +     dev_t                   rdev,
>> +     xfs_inode_t             **ipp)
>
> Please use struct xfs_mount and struct xfs_inode for any new code.
>
>> +     /*
>> +      * Initially assume that the file does not exist and
>> +      * reserve the resources for that case.  If that is not
>> +      * the case we'll drop the one we have and get a more
>> +      * appropriate transaction later.
>> +      */
>
> I can't see how this comment makes any sense.
>
>> +     tres = &M_RES(mp)->tr_create_tmpfile;
>> +     error = xfs_trans_reserve(tp, tres, resblks, 0);
>> +     if (error == ENOSPC) {
>> +             /* flush outstanding delalloc blocks and retry */
>> +             xfs_flush_inodes(mp);
>> +             error = xfs_trans_reserve(tp, tres, resblks, 0);
>> +     }
>> +     if (error == ENOSPC) {
>> +             /* No space at all so try a "no-allocation" reservation */
>> +             resblks = 0;
>> +             error = xfs_trans_reserve(tp, tres, 0, 0);
>> +     }
>
> Please factor this into a new xfs_trans_reserver_create helper (better
> names welcome of course).
> similar.
>
>> +     /*
>> +      * Reserve disk quota and the inode.
>> +      */
>
> I don't think that comment adds a whole lot of value.  (Same for the
> other quota comment above).
>
>> +     /*
>> +      * A newly created regular or special file just has one directory
>> +      * entry pointing to them, but a directory also the "." entry
>> +      * pointing to itself.
>> +      */
>
> Given that we only create regular files here the comment can be removed.
>
>>
>> +STATIC uint
>> +xfs_calc_icreate_tmpfile_reservation(xfs_mount_t *mp)
>> +{
>> +     return XFS_DQUOT_LOGRES(mp) +
>> +             xfs_calc_icreate_resv_alloc(mp) +
>> +             xfs_calc_inode_res(mp, 1) +
>> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
>> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>> +}
>> +
>> +STATIC uint
>> +__xfs_calc_create_tmpfile_reservation(
>> +     struct xfs_mount        *mp)
>> +{
>> +     return XFS_DQUOT_LOGRES(mp) +
>> +             xfs_calc_create_resv_alloc(mp) +
>> +             xfs_calc_inode_res(mp, 1) +
>> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
>> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>> +}
>> +
>> +STATIC uint
>> +xfs_calc_create_tmpfile_reservation(
>> +     struct xfs_mount        *mp)
>> +{
>> +     if (xfs_sb_version_hascrc(&mp->m_sb))
>> +             return xfs_calc_icreate_tmpfile_reservation(mp);
>> +     return __xfs_calc_create_tmpfile_reservation(mp);
>
> Shouldn't we name this xfs_calc_create_tmpfile_reservation_v4 and _v5
> or no postix and _crc?  Either way the double underscore naming looks
> confusing.
It follows up with the current naming style of regular file
reservation functions.
After we adopt Dave's suggestion, this issue will disappear.

>



-- 
Regards,

Zhi Yong Wu

<Prev in Thread] Current Thread [Next in Thread>