From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0A05E225417 for ; Wed, 4 Mar 2026 00:59:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772586000; cv=none; b=VYW7BKsy5UYu6XRRSkLOGhoGuvhmLh8IFwMpMSa5CutJhnJ5RRV6eA6dzoA7FBeu3a2lkC9LwBAhaPJqAH2p87PR6DO+kfj2TbPtcy1YWuHN2MpaNB2xaugFQKUK4yUvUrVQBypfAQ4aSx3H5gX+1V9WKhALmh9Hf7ZGLtkYLfU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772586000; c=relaxed/simple; bh=oNemJ1q7LWiX7s8ycw3g5Ds2US7FjZD3R2aycP2kwmc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=m01r7qRTkhOq4uF8rO8HNi682dVWbeTetmpp9+lJKbNpCvqIVIpoKal/ew99Z35MQt/Qhcc+5mhFFthhLMhkmsA8bS+FmzJ5YEUkJk3Lh6JxuR4KdDGY+lK9MPHvPdYz8aKvtum6bhi5718L6jkg8nMRW3CP8EGfUUdYX3HkFt0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=zabbo.net; spf=pass smtp.mailfrom=zabbo.net; dkim=pass (1024-bit key) header.d=zabbo.net header.i=@zabbo.net header.b=pDfRYvR4; arc=none smtp.client-ip=209.85.216.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=zabbo.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=zabbo.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=zabbo.net header.i=@zabbo.net header.b="pDfRYvR4" Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-35691a231a7so3866170a91.3 for ; Tue, 03 Mar 2026 16:59:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=zabbo.net; s=google; t=1772585998; x=1773190798; darn=lists.linux.dev; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=PzY2pWdLUyAuMyg/je+VUibW2TApxtcbGyqmEIsftgg=; b=pDfRYvR4pgSxXwIeiehRKOCmLnkl+l6hIHGKyZBx9aTtme562TFmEExPm3k2oTBdsZ xPr4vB7Ftu9ZY274qRAYodsaAZSCcfxuNau0urNRzE2F296ji6zahm61EEbvmj7e1FRn oPXOz/DO/zdIf8EVjxzl5BWhOb6V3jW+gr02A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1772585998; x=1773190798; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=PzY2pWdLUyAuMyg/je+VUibW2TApxtcbGyqmEIsftgg=; b=I1E8b6KH8GidJFn3/XrjpDi/R1ARs4FsPwKpNJikqDK23Uh96G1c6tKlNEfU5ZW1zT sTdRjSjorfPXmvFLfWhgkBH4roVLd7psAUnGLtuUmEr3srUq714pC+ePWaVpKsbtB85/ ehfTlnYWhmTpq3DDbpT1/Q34kzoNJYRDmXGySf0KELK/ldnSIkEcvQMuLawunDM1B/ks 2wRjOc4YQ9WgAH1/m6WD+QkrT/K6NhZauHc1YR2sNLxt5rf3qw93tSD/NGansQTV79nA Qu1/HtnedzCaaZ4FJbB324gAW2cOCSEdWR41/ZGrgXx3RGRKwdG4z5VYQqs/AmBP8lS0 5Gqw== X-Gm-Message-State: AOJu0YwCd7muxhr4v8H9LsQ4hOWvg7b6wNn9RJ7RCUIQRbqGnSoxrb1D ClXdphUpYCKW4WUkKD0QK0KMZNEdeXLmPU6g2YWnnUkG+f/oPkXE8qpZqEbUeDC7arI= X-Gm-Gg: ATEYQzzYTkIjVIJrI182mOympyHbakdwyvnUTB/LrSxbe5ygjhjqtX2s6azYGczD18S nySYUbmuxgZ7nk8K9otnuBWxAcQ7tYgNaZTtLkXbWqFNssh/I5IMfHYL/TFu12s9px5D6Mfg5fz j5QGVmC/raVKR5hulOKbXbAosswEDZczDWlJ7uU13lvxFWvIG8tSGm2wzhQ6w20RKoDyOkuSRu3 uZl6VexmJmvKgu91yjWrGF1PkFMkD2J3+errrzS3hDbAV2tzSaMljkLVl6bRbpjhOEz/0ljo9Xx lU+b9Sixx3TR4WpEDeVMxynTLnyWCun7HC9NJa2wjRCKWi2P9eyVSaLuRIf3NcG0S5di1OIIWMB n2l4spikMdotfqlYo9Qe04IbvKlKBSpxCGAJFIKOZqzyqVw5XiRfi2xglylN4f8fq9Z6TNbRMC1 dlb5Xu+0lG8g== X-Received: by 2002:a17:90b:1e0b:b0:359:2d1c:9206 with SMTP id 98e67ed59e1d1-359a6a97876mr365226a91.33.1772585998217; Tue, 03 Mar 2026 16:59:58 -0800 (PST) Received: from localhost ([50.39.133.72]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-3599bcd81e8sm1846592a91.5.2026.03.03.16.59.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Mar 2026 16:59:57 -0800 (PST) Date: Tue, 3 Mar 2026 16:59:57 -0800 From: Zach Brown To: Chris Kirby Cc: rpdfs-devel@lists.linux.dev Subject: Re: [PATCH 1/1] rpdfs: Initial extended attribute support Message-ID: <20260304005957.GC3280611@localhost.localdomain> References: Precedence: bulk X-Mailing-List: rpdfs-devel@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Tue, Mar 03, 2026 at 09:20:52AM -0600, Chris Kirby wrote: > Initial extended attribute support Just a few cleanups.. > +#define RPDFS_XATTR_MAX_NAME_LEN RPDFS_NAME_MAX I suppose this should go in format-block.h by the other xattr constants. (And like the dirent _NAME_MAX equivalent). > + if (copy_value) { > + char *dest; I have a really old habit to not declare local variables in blocks from old gcc bugs that used to blow out the stack. (I think due to larger alignments.. it's been so long!) Anyway, if possible, declare everything up top. > + dest = (char *)kx + offsetof(struct key_xattr, > + xattr.name[name_len]); > + memcpy(dest, value, size); But in this case do we still need this single-use indirection after the flex array fixes? If we do, so be it, but if this is left over from trying to avoid the array overflow warnings before the flex union fix then it'd be nice to clean this (and the other instances) up. > +static int add_xattr_item_cb(struct rpdfs_fs_info *rfi, > + struct rpdfs_btree_item_args *a, > + struct rpdfs_btree_item_args *b, > + struct rpdfs_btree_item_args *ins, > + void *arg) > +{ > + struct key_xattr *kx = arg; > + > + if (a && xattr_key_hash(&kx->key) == xattr_key_hash(&a->key)) > + return -EEXIST; We all knew this, but to be explicit: these patterns don't make sense for the xattrs that have a unique lsq in the key. These come from the dirents that only allow a single collision bit so that they can be done in one call to a handful of items in one block. When we have a full 64bits of collision the potentially matching entries can span lots of items and blocks in the btree. But the btree is going away, and this makes it an easy to follow derivation from the dirents, so sure, let's go with this for now. It won't matter before we replace the btree and all of this changes. > + do { > + have_old = false; > + > + ret = rpdfs_inode_txn_prepare(rfi, &txn, inode, RBAF_WRITE); > + if (ret == 0) { > + ret = rpdfs_btree_txn_prepare_lookup(rfi, &txn, > + &RPDFS_I(inode)->xattrs, > + &old_kx->key, > + lookup_xattr_cb, > + old_kx); > + > + /* lookup returns the xattr size if it finds the name */ > + if (ret >= 0) { > + have_old = true; > + ret = 0; > + } > + > + /* > + * It's an error to specify XATTR_REPLACE if the name > + * doesn't already exist. > + */ > + if (!have_old) { > + if (flags & XATTR_REPLACE) > + ret = -ENODATA; > + else > + ret = 0; > + } > + > + /* > + * It's an error to specify XATTR_CREATE if the name > + * already exists. > + */ > + if (ret == 0 && have_old && (flags & XATTR_CREATE)) > + ret = -EEXIST; > + > + if (ret == 0 && have_old) { > + ret = prepare_delete_xattr(rfi, &txn, inode, > + old_kx); > + } > + } > + > + if (ret == 0 && value != NULL) { > + xattr_key_set_uniq(new_kx, ri->xattr_creates); > + > + ret = prepare_add_xattr(rfi, &txn, inode, new_kx); > + } > + } while (rpdfs_txn_retry(rfi, &txn, &ret)); Oof, that's a lot to be in the retry block. If it's more than a handful of lines of obvious cascading error checking, can you pop it up in a helper function? > + creates = __le64_to_cpu(ri->xattr_creates) + 1; > + > + if (have_old) > + apply_delete_xattr(rfi, &txn, inode, old_kx); > + > + if (value) { > + apply_add_xattr(rfi, &txn, inode, new_kx); > + ri->xattr_creates = cpu_to_le64(creates); Drop the "creates" that might not have been used and increment directly with le64_add_cpu(&ri->xattr_creates, 1); - z