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 _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
