On Tue, Dec 24, 2019 at 09:21:49PM -0700, Allison Collins wrote: > On 12/24/19 5:18 AM, Christoph Hellwig wrote: > > On Wed, Dec 11, 2019 at 09:15:03PM -0700, Allison Collins wrote: > > > From: Allison Henderson <allison.henderson@xxxxxxxxxx> > > > > > > This patch adds a new functions to check for the existence of > > > an attribute. Subroutines are also added to handle the cases > > > of leaf blocks, nodes or shortform. Common code that appears > > > in existing attr add and remove functions have been factored > > > out to help reduce the appearance of duplicated code. We will > > > need these routines later for delayed attributes since delayed > > > operations cannot return error codes. > > > > Can you explain why we need the ahead of time check? The first > > operation should be able to still return an error, and doing > > a separate check instead of letting the actual operation fail > > gracefully is more expensive, and also creates a lot of additional > > code. As is I can't say I like the direction at all. > > > > This one I can answer quickly: later when we get into delayed attributes, > this will get called from xfs_defer_finish_noroll as part of a .finish_item > call back. If these callbacks return anything other than 0 or -EAGAIN, it > causes a shutdown. When does this happen, exactly? Are you saying that during log recovery, we can end up replaying a delayed attr log item that hits ENOATTR/EEXIST somewhere and passes that out, which causes log recovery to fail? > Which is not what we would want for example: when the > user tries to rename a non-existent attribute. The error code needs to go > back up. So we check for things like that before starting a delayed > operation. Hope that helps. Thanks! ...because as far as requests from user programs goes, we should be doing all these precondition checks after allocating a transaction and ILOCKing the inode, so that we can send the error code back to userspace without cancelling a dirty transaction. (I dunno, am I misunderstanding here?) > Allison
