From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f68.google.com ([209.85.218.68]:32925 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750861AbcLVFyi (ORCPT ); Thu, 22 Dec 2016 00:54:38 -0500 Received: by mail-oi0-f68.google.com with SMTP id f201so31190418oib.0 for ; Wed, 21 Dec 2016 21:54:38 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20161221225605.qcgpwkyrugwmjnza@thunk.org> References: <1482178268-22883-1-git-send-email-amir73il@gmail.com> <1482178268-22883-12-git-send-email-amir73il@gmail.com> <20161220002832.GB7297@birch.djwong.org> <20161221052317.GA4758@dastard> <20161221150619.hkgsqwcdzkqlum4v@thunk.org> <20161221225605.qcgpwkyrugwmjnza@thunk.org> From: Amir Goldstein Date: Thu, 22 Dec 2016 07:54:37 +0200 Message-ID: Subject: Re: [RFC][PATCH 11/11] xfs: use common file type conversion To: "Theodore Ts'o" Cc: Dave Chinner , "Darrick J. Wong" , Jan Kara , Chris Mason , Boaz Harrosh , Jaegeuk Kim , Ryusuke Konishi , Mark Fasheh , Evgeniy Dushistov , Miklos Szeredi , Al Viro , linux-fsdevel Content-Type: text/plain; charset=UTF-8 Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Dec 22, 2016 at 12:56 AM, Theodore Ts'o wrote: > On Wed, Dec 21, 2016 at 06:37:42PM +0200, Amir Goldstein wrote: >> >> Right. never claimed that saving data segment space is the issue >> here. To be completely honest, I started looking at ways to optimize >> whiteout iteration and was appalled to see all that code copy&paste, >> so went on an OCD cleanup spree. >> >> So it is really a lot more about clumsiness of the current code then >> about saving actual lines on code or space in data segment. >> >> Perhaps I should have settled with defining this common section: > >> +#define DT_WHT 14 > > What are you trying to accomplish here? Unless we actually encoding > DT_WHT into the on-disk format, it's not really going to buy you much. > > And if you are going to encode this into the on-disk format, each file > system is going to have to set some kind of flag in the superblock to > indicate whether it's doing this new thing or not. And this is > *precisely* why Dave and Darrick are objecting --- if we are going to > make any kind of file system encoding change, the file system has to > know about it. > Ted, There is a bit of a confusion here. Although I would have liked to be able to add DT_WHT to dirent I found there is no east way to do it without adding fs feature flags. I did not add the DT_WHT definion, I kept it as is just moved from fs.h to file_type.h because some fs (e.g. xfs) still use this ghost define. >> Note that all those file system copy&pasted a potential bug. >> >> This is an array of size 15: >> >> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = { >> >> and it is always addressed this way: >> >> ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT]; >> >> Which *can* try to address ext2_type_by_mode[15] >> >> Now, can you say for certain that you can never get a malformed i_mode >> with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from >> some code calling into VFS functions, which do not sanity check the >> file type of the mode argument. > > It's not a problem and not a bug, because we only assign the file type > when the inode is created, and at that point, the i_mode is set by the > *kernel*, and not by the on-disk encoding. > Not entirley true. On rename some fs set dentry type of target by converting the mode of the renamed object. I don't remember what ext4 does, but I think it does the same, so malformed imode on disk could potentially get you there. > And BTW this is why DT_WHT makes no sense. We the DT encodings come > from the directory entry, and *never* come from the inode i_mode read > from disk --- since at the time when we do the readdir(2), the inode > may not have been read into memory. So we can't add DT_WHT unless we > also change the on-disk encoding of the directory entry on a file > system by file system basis. > True. it makes no sense at all, but we can't remove it. XFS can decide to start treating it as an error if found on disk, but that's another story. Amir.