* [PATCH net-next v8 3/3] selftests: add MSG_ZEROCOPY msg_control notification test
From: zijianzhang @ 2024-07-30 18:41 UTC (permalink / raw)
To: netdev
Cc: linux-api, willemdebruijn.kernel, almasrymina, edumazet, davem,
kuba, pabeni, dsahern, axboe, shuah, linux-kselftest, cong.wang,
xiaochun.lu, Zijian Zhang
In-Reply-To: <20240730184120.4089835-1-zijianzhang@bytedance.com>
From: Zijian Zhang <zijianzhang@bytedance.com>
We update selftests/net/msg_zerocopy.c to accommodate the new mechanism,
cfg_notification_limit has the same semantics for both methods. Test
results are as follows, we update skb_orphan_frags_rx to the same as
skb_orphan_frags to support zerocopy in the localhost test.
cfg_notification_limit = 1, both method get notifications after 1 calling
of sendmsg. In this case, the new method has around 17% cpu savings in TCP
and 23% cpu savings in UDP.
+---------------------+---------+---------+---------+---------+
| Test Type / Protocol| TCP v4 | TCP v6 | UDP v4 | UDP v6 |
+---------------------+---------+---------+---------+---------+
| ZCopy (MB) | 7523 | 7706 | 7489 | 7304 |
+---------------------+---------+---------+---------+---------+
| New ZCopy (MB) | 8834 | 8993 | 9053 | 9228 |
+---------------------+---------+---------+---------+---------+
| New ZCopy / ZCopy | 117.42% | 116.70% | 120.88% | 126.34% |
+---------------------+---------+---------+---------+---------+
cfg_notification_limit = 32, both get notifications after 32 calling of
sendmsg, which means more chances to coalesce notifications, and less
overhead of poll + recvmsg for the original method. In this case, the new
method has around 7% cpu savings in TCP and slightly better cpu usage in
UDP. In the env of selftest, notifications of TCP are more likely to be
out of order than UDP, it's easier to coalesce more notifications in UDP.
The original method can get one notification with range of 32 in a recvmsg
most of the time. In TCP, most notifications' range is around 2, so the
original method needs around 16 recvmsgs to get notified in one round.
That's the reason for the "New ZCopy / ZCopy" diff in TCP and UDP here.
+---------------------+---------+---------+---------+---------+
| Test Type / Protocol| TCP v4 | TCP v6 | UDP v4 | UDP v6 |
+---------------------+---------+---------+---------+---------+
| ZCopy (MB) | 8842 | 8735 | 10072 | 9380 |
+---------------------+---------+---------+---------+---------+
| New ZCopy (MB) | 9366 | 9477 | 10108 | 9385 |
+---------------------+---------+---------+---------+---------+
| New ZCopy / ZCopy | 106.00% | 108.28% | 100.31% | 100.01% |
+---------------------+---------+---------+---------+---------+
In conclusion, when notification interval is small or notifications are
hard to be coalesced, the new mechanism is highly recommended. Otherwise,
the performance gain from the new mechanism is very limited.
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Signed-off-by: Xiaochun Lu <xiaochun.lu@bytedance.com>
---
tools/testing/selftests/net/msg_zerocopy.c | 101 ++++++++++++++++++--
tools/testing/selftests/net/msg_zerocopy.sh | 1 +
2 files changed, 95 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/net/msg_zerocopy.c b/tools/testing/selftests/net/msg_zerocopy.c
index 7ea5fb28c93d..cf227f0011b5 100644
--- a/tools/testing/selftests/net/msg_zerocopy.c
+++ b/tools/testing/selftests/net/msg_zerocopy.c
@@ -66,6 +66,10 @@
#define SO_ZEROCOPY 60
#endif
+#ifndef SCM_ZC_NOTIFICATION
+#define SCM_ZC_NOTIFICATION 78
+#endif
+
#ifndef SO_EE_CODE_ZEROCOPY_COPIED
#define SO_EE_CODE_ZEROCOPY_COPIED 1
#endif
@@ -74,6 +78,14 @@
#define MSG_ZEROCOPY 0x4000000
#endif
+#define ZC_INFO_ARR_SIZE (ZC_NOTIFICATION_MAX * sizeof(struct zc_info_elem))
+#define ZC_INFO_SIZE (sizeof(struct zc_info) + ZC_INFO_ARR_SIZE)
+
+enum notification_type {
+ MSG_ZEROCOPY_NOTIFY_ERRQUEUE = 1,
+ MSG_ZEROCOPY_NOTIFY_SENDMSG = 2,
+};
+
static int cfg_cork;
static bool cfg_cork_mixed;
static int cfg_cpu = -1; /* default: pin to last cpu */
@@ -86,7 +98,7 @@ static int cfg_runtime_ms = 4200;
static int cfg_verbose;
static int cfg_waittime_ms = 500;
static int cfg_notification_limit = 32;
-static bool cfg_zerocopy;
+static enum notification_type cfg_zerocopy;
static socklen_t cfg_alen;
static struct sockaddr_storage cfg_dst_addr;
@@ -97,6 +109,8 @@ static long packets, bytes, completions, expected_completions;
static int zerocopied = -1;
static uint32_t next_completion;
static uint32_t sends_since_notify;
+static char zc_ckbuf[CMSG_SPACE(ZC_INFO_SIZE)];
+static bool added_zcopy_info;
static unsigned long gettimeofday_ms(void)
{
@@ -182,7 +196,26 @@ static void add_zcopy_cookie(struct msghdr *msg, uint32_t cookie)
memcpy(CMSG_DATA(cm), &cookie, sizeof(cookie));
}
-static bool do_sendmsg(int fd, struct msghdr *msg, bool do_zerocopy, int domain)
+static void add_zcopy_info(struct msghdr *msg)
+{
+ struct zc_info *zc_info;
+ struct cmsghdr *cm;
+
+ if (!msg->msg_control)
+ error(1, errno, "NULL user arg");
+ cm = (struct cmsghdr *)msg->msg_control;
+ cm->cmsg_len = CMSG_LEN(ZC_INFO_SIZE);
+ cm->cmsg_level = SOL_SOCKET;
+ cm->cmsg_type = SCM_ZC_NOTIFICATION;
+
+ zc_info = (struct zc_info *)CMSG_DATA(cm);
+ zc_info->size = ZC_NOTIFICATION_MAX;
+
+ added_zcopy_info = true;
+}
+
+static bool do_sendmsg(int fd, struct msghdr *msg,
+ enum notification_type do_zerocopy, int domain)
{
int ret, len, i, flags;
static uint32_t cookie;
@@ -200,6 +233,12 @@ static bool do_sendmsg(int fd, struct msghdr *msg, bool do_zerocopy, int domain)
msg->msg_controllen = CMSG_SPACE(sizeof(cookie));
msg->msg_control = (struct cmsghdr *)ckbuf;
add_zcopy_cookie(msg, ++cookie);
+ } else if (do_zerocopy == MSG_ZEROCOPY_NOTIFY_SENDMSG &&
+ sends_since_notify + 1 >= cfg_notification_limit) {
+ memset(&msg->msg_control, 0, sizeof(msg->msg_control));
+ msg->msg_controllen = CMSG_SPACE(ZC_INFO_SIZE);
+ msg->msg_control = (struct cmsghdr *)zc_ckbuf;
+ add_zcopy_info(msg);
}
}
@@ -218,7 +257,7 @@ static bool do_sendmsg(int fd, struct msghdr *msg, bool do_zerocopy, int domain)
if (do_zerocopy && ret)
expected_completions++;
}
- if (do_zerocopy && domain == PF_RDS) {
+ if (msg->msg_control) {
msg->msg_control = NULL;
msg->msg_controllen = 0;
}
@@ -466,6 +505,44 @@ static void do_recv_completions(int fd, int domain)
sends_since_notify = 0;
}
+static void do_recv_completions2(void)
+{
+ struct cmsghdr *cm = (struct cmsghdr *)zc_ckbuf;
+ struct zc_info *zc_info;
+ __u32 hi, lo, range;
+ __u8 zerocopy;
+ int i;
+
+ zc_info = (struct zc_info *)CMSG_DATA(cm);
+ for (i = 0; i < zc_info->size; i++) {
+ hi = zc_info->arr[i].hi;
+ lo = zc_info->arr[i].lo;
+ zerocopy = zc_info->arr[i].zerocopy;
+ range = hi - lo + 1;
+
+ if (cfg_verbose && lo != next_completion)
+ fprintf(stderr, "gap: %u..%u does not append to %u\n",
+ lo, hi, next_completion);
+ next_completion = hi + 1;
+
+ if (zerocopied == -1) {
+ zerocopied = zerocopy;
+ } else if (zerocopied != zerocopy) {
+ fprintf(stderr, "serr: inconsistent\n");
+ zerocopied = zerocopy;
+ }
+
+ completions += range;
+ sends_since_notify -= range;
+
+ if (cfg_verbose >= 2)
+ fprintf(stderr, "completed: %u (h=%u l=%u)\n",
+ range, hi, lo);
+ }
+
+ added_zcopy_info = false;
+}
+
/* Wait for all remaining completions on the errqueue */
static void do_recv_remaining_completions(int fd, int domain)
{
@@ -553,11 +630,16 @@ static void do_tx(int domain, int type, int protocol)
else
do_sendmsg(fd, &msg, cfg_zerocopy, domain);
- if (cfg_zerocopy && sends_since_notify >= cfg_notification_limit)
+ if (cfg_zerocopy == MSG_ZEROCOPY_NOTIFY_ERRQUEUE &&
+ sends_since_notify >= cfg_notification_limit)
do_recv_completions(fd, domain);
+ if (cfg_zerocopy == MSG_ZEROCOPY_NOTIFY_SENDMSG &&
+ added_zcopy_info)
+ do_recv_completions2();
+
while (!do_poll(fd, POLLOUT)) {
- if (cfg_zerocopy)
+ if (cfg_zerocopy == MSG_ZEROCOPY_NOTIFY_ERRQUEUE)
do_recv_completions(fd, domain);
}
@@ -715,7 +797,7 @@ static void parse_opts(int argc, char **argv)
cfg_payload_len = max_payload_len;
- while ((c = getopt(argc, argv, "46c:C:D:i:l:mp:rs:S:t:vz")) != -1) {
+ while ((c = getopt(argc, argv, "46c:C:D:i:l:mnp:rs:S:t:vz")) != -1) {
switch (c) {
case '4':
if (cfg_family != PF_UNSPEC)
@@ -749,6 +831,9 @@ static void parse_opts(int argc, char **argv)
case 'm':
cfg_cork_mixed = true;
break;
+ case 'n':
+ cfg_zerocopy = MSG_ZEROCOPY_NOTIFY_SENDMSG;
+ break;
case 'p':
cfg_port = strtoul(optarg, NULL, 0);
break;
@@ -768,7 +853,7 @@ static void parse_opts(int argc, char **argv)
cfg_verbose++;
break;
case 'z':
- cfg_zerocopy = true;
+ cfg_zerocopy = MSG_ZEROCOPY_NOTIFY_ERRQUEUE;
break;
}
}
@@ -779,6 +864,8 @@ static void parse_opts(int argc, char **argv)
error(1, 0, "-D <server addr> required for PF_RDS\n");
if (!cfg_rx && !saddr)
error(1, 0, "-S <client addr> required for PF_RDS\n");
+ if (cfg_zerocopy == MSG_ZEROCOPY_NOTIFY_SENDMSG)
+ error(1, 0, "PF_RDS does not support ZC_NOTIF_SENDMSG");
}
setup_sockaddr(cfg_family, daddr, &cfg_dst_addr);
setup_sockaddr(cfg_family, saddr, &cfg_src_addr);
diff --git a/tools/testing/selftests/net/msg_zerocopy.sh b/tools/testing/selftests/net/msg_zerocopy.sh
index 89c22f5320e0..022a6936d86f 100755
--- a/tools/testing/selftests/net/msg_zerocopy.sh
+++ b/tools/testing/selftests/net/msg_zerocopy.sh
@@ -118,4 +118,5 @@ do_test() {
do_test "${EXTRA_ARGS}"
do_test "-z ${EXTRA_ARGS}"
+do_test "-n ${EXTRA_ARGS}"
echo ok
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v8 2/3] sock: add MSG_ZEROCOPY notification mechanism based on msg_control
From: zijianzhang @ 2024-07-30 18:41 UTC (permalink / raw)
To: netdev
Cc: linux-api, willemdebruijn.kernel, almasrymina, edumazet, davem,
kuba, pabeni, dsahern, axboe, shuah, linux-kselftest, cong.wang,
xiaochun.lu, Zijian Zhang
In-Reply-To: <20240730184120.4089835-1-zijianzhang@bytedance.com>
From: Zijian Zhang <zijianzhang@bytedance.com>
The MSG_ZEROCOPY flag enables copy avoidance for socket send calls.
However, zerocopy is not a free lunch. Apart from the management of user
pages, the combination of poll + recvmsg to receive notifications incurs
unignorable overhead in the applications. We try to mitigate this overhead
with a new notification mechanism based on msg_control. Leveraging the
general framework to copy cmsgs to the user space, we copy zerocopy
notifications to the user upon returning of sendmsgs.
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Signed-off-by: Xiaochun Lu <xiaochun.lu@bytedance.com>
---
arch/alpha/include/uapi/asm/socket.h | 2 +
arch/mips/include/uapi/asm/socket.h | 2 +
arch/parisc/include/uapi/asm/socket.h | 2 +
arch/sparc/include/uapi/asm/socket.h | 2 +
include/linux/socket.h | 2 +-
include/uapi/asm-generic/socket.h | 2 +
include/uapi/linux/socket.h | 23 +++++++++
net/core/sock.c | 72 +++++++++++++++++++++++++--
8 files changed, 102 insertions(+), 5 deletions(-)
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
index e94f621903fe..7c32d9dbe47f 100644
--- a/arch/alpha/include/uapi/asm/socket.h
+++ b/arch/alpha/include/uapi/asm/socket.h
@@ -140,6 +140,8 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SCM_ZC_NOTIFICATION 78
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64
diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
index 60ebaed28a4c..3f7fade998cb 100644
--- a/arch/mips/include/uapi/asm/socket.h
+++ b/arch/mips/include/uapi/asm/socket.h
@@ -151,6 +151,8 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SCM_ZC_NOTIFICATION 78
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64
diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
index be264c2b1a11..77f5bee0fdc9 100644
--- a/arch/parisc/include/uapi/asm/socket.h
+++ b/arch/parisc/include/uapi/asm/socket.h
@@ -132,6 +132,8 @@
#define SO_PASSPIDFD 0x404A
#define SO_PEERPIDFD 0x404B
+#define SCM_ZC_NOTIFICATION 0x404C
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64
diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
index 682da3714686..eb44fc515b45 100644
--- a/arch/sparc/include/uapi/asm/socket.h
+++ b/arch/sparc/include/uapi/asm/socket.h
@@ -133,6 +133,8 @@
#define SO_PASSPIDFD 0x0055
#define SO_PEERPIDFD 0x0056
+#define SCM_ZC_NOTIFICATION 0x0057
+
#if !defined(__KERNEL__)
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 40173c919d0f..71e3c6ebfed5 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -171,7 +171,7 @@ static inline struct cmsghdr * cmsg_nxthdr (struct msghdr *__msg, struct cmsghdr
static inline bool cmsg_copy_to_user(struct cmsghdr *__cmsg)
{
- return 0;
+ return __cmsg->cmsg_type == SCM_ZC_NOTIFICATION;
}
static inline size_t msg_data_left(struct msghdr *msg)
diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
index 8ce8a39a1e5f..02e9159c7944 100644
--- a/include/uapi/asm-generic/socket.h
+++ b/include/uapi/asm-generic/socket.h
@@ -135,6 +135,8 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SCM_ZC_NOTIFICATION 78
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
diff --git a/include/uapi/linux/socket.h b/include/uapi/linux/socket.h
index d3fcd3b5ec53..b5b5fa9febb1 100644
--- a/include/uapi/linux/socket.h
+++ b/include/uapi/linux/socket.h
@@ -2,6 +2,8 @@
#ifndef _UAPI_LINUX_SOCKET_H
#define _UAPI_LINUX_SOCKET_H
+#include <linux/types.h>
+
/*
* Desired design of maximum size and alignment (see RFC2553)
*/
@@ -35,4 +37,25 @@ struct __kernel_sockaddr_storage {
#define SOCK_TXREHASH_DISABLED 0
#define SOCK_TXREHASH_ENABLED 1
+#define ZC_NOTIFICATION_MAX 16
+
+/*
+ * A zc_info_elem represents a completion notification for sendmsgs in range
+ * lo to high, zerocopy represents whether the underlying transmission is
+ * zerocopy or not.
+ */
+struct zc_info_elem {
+ __u32 lo;
+ __u32 hi;
+ __u8 zerocopy;
+};
+
+/*
+ * zc_info is the struct used for the SCM_ZC_NOTIFICATION control message.
+ */
+struct zc_info {
+ __u32 size; /* size of the zc_info_elem arr */
+ struct zc_info_elem arr[];
+};
+
#endif /* _UAPI_LINUX_SOCKET_H */
diff --git a/net/core/sock.c b/net/core/sock.c
index b2cbe753af1d..37b1b12623ee 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1481,10 +1481,12 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
ret = -EOPNOTSUPP;
}
if (!ret) {
- if (val < 0 || val > 1)
+ if (val < 0 || val > 1) {
ret = -EINVAL;
- else
+ } else {
sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool);
+ static_branch_enable(&tx_copy_cmsg_to_user_key);
+ }
}
break;
@@ -2826,8 +2828,8 @@ struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
}
EXPORT_SYMBOL(sock_alloc_send_pskb);
-int __sock_cmsg_send(struct sock *sk, struct msghdr *msg __always_unused,
- struct cmsghdr *cmsg, struct sockcm_cookie *sockc)
+int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
+ struct sockcm_cookie *sockc)
{
u32 tsflags;
@@ -2863,6 +2865,68 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg __always_unused,
case SCM_RIGHTS:
case SCM_CREDENTIALS:
break;
+ case SCM_ZC_NOTIFICATION: {
+ struct zc_info *zc = CMSG_DATA(cmsg);
+ struct sk_buff_head *q, local_q;
+ int cmsg_data_len, i = 0;
+ unsigned long flags;
+ struct sk_buff *skb;
+
+ if (!sock_flag(sk, SOCK_ZEROCOPY) || sk->sk_family == PF_RDS)
+ return -EINVAL;
+
+ cmsg_data_len = cmsg->cmsg_len - sizeof(struct cmsghdr);
+ if (cmsg_data_len < sizeof(struct zc_info))
+ return -EINVAL;
+
+ if (zc->size > ZC_NOTIFICATION_MAX ||
+ (cmsg_data_len - sizeof(struct zc_info)) !=
+ (zc->size * sizeof(struct zc_info_elem)))
+ return -EINVAL;
+
+ q = &sk->sk_error_queue;
+ skb_queue_head_init(&local_q);
+
+ /* Get zerocopy error messages from sk_error_queue, and add them
+ * to a local queue for later processing. This minimizes the
+ * code while the spinlock is held and irq is disabled.
+ */
+ spin_lock_irqsave(&q->lock, flags);
+ skb = skb_peek(q);
+ while (skb && i < zc->size) {
+ struct sk_buff *skb_next = skb_peek_next(skb, q);
+ struct sock_exterr_skb *serr = SKB_EXT_ERR(skb);
+
+ if (serr->ee.ee_errno != 0 ||
+ serr->ee.ee_origin != SO_EE_ORIGIN_ZEROCOPY) {
+ skb = skb_next;
+ continue;
+ }
+
+ __skb_unlink(skb, q);
+ __skb_queue_tail(&local_q, skb);
+ skb = skb_next;
+ i++;
+ }
+ spin_unlock_irqrestore(&q->lock, flags);
+
+ i = 0;
+ while ((skb = skb_peek(&local_q)) != NULL) {
+ struct sock_exterr_skb *serr = SKB_EXT_ERR(skb);
+
+ zc->arr[i].hi = serr->ee.ee_data;
+ zc->arr[i].lo = serr->ee.ee_info;
+ zc->arr[i].zerocopy = !(serr->ee.ee_code
+ & SO_EE_CODE_ZEROCOPY_COPIED);
+ __skb_unlink(skb, &local_q);
+ consume_skb(skb);
+ i++;
+ }
+
+ zc->size = i;
+ msg->msg_control_copy_to_user = true;
+ break;
+ }
default:
return -EINVAL;
}
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v8 1/3] sock: support copying cmsgs to the user space in sendmsg
From: zijianzhang @ 2024-07-30 18:41 UTC (permalink / raw)
To: netdev
Cc: linux-api, willemdebruijn.kernel, almasrymina, edumazet, davem,
kuba, pabeni, dsahern, axboe, shuah, linux-kselftest, cong.wang,
xiaochun.lu, Zijian Zhang
In-Reply-To: <20240730184120.4089835-1-zijianzhang@bytedance.com>
From: Zijian Zhang <zijianzhang@bytedance.com>
Users can pass msg_control as a placeholder to recvmsg, and get some info
from the kernel upon returning of it, but it's not available for sendmsg.
Recvmsg uses put_cmsg to copy info back to the user, while ____sys_sendmsg
creates a kernel copy of msg_control and passes that to the callees,
put_cmsg in sendmsg path will write into this kernel buffer.
If users want to get info after returning of sendmsg, they typically have
to call recvmsg on the MSG_ERRQUEUE of the socket, incurring extra system
call overhead. This commit supports copying cmsg from the kernel space to
the user space upon returning of sendmsg to mitigate this overhead.
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Signed-off-by: Xiaochun Lu <xiaochun.lu@bytedance.com>
---
include/linux/socket.h | 8 ++++++
include/net/sock.h | 2 +-
net/core/sock.c | 6 ++--
net/ipv4/ip_sockglue.c | 2 +-
net/ipv6/datagram.c | 2 +-
net/socket.c | 63 ++++++++++++++++++++++++++++++++++++++----
6 files changed, 72 insertions(+), 11 deletions(-)
diff --git a/include/linux/socket.h b/include/linux/socket.h
index df9cdb8bbfb8..40173c919d0f 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -71,6 +71,7 @@ struct msghdr {
void __user *msg_control_user;
};
bool msg_control_is_user : 1;
+ bool msg_control_copy_to_user : 1;
bool msg_get_inq : 1;/* return INQ after receive */
unsigned int msg_flags; /* flags on received message */
__kernel_size_t msg_controllen; /* ancillary data buffer length */
@@ -168,6 +169,11 @@ static inline struct cmsghdr * cmsg_nxthdr (struct msghdr *__msg, struct cmsghdr
return __cmsg_nxthdr(__msg->msg_control, __msg->msg_controllen, __cmsg);
}
+static inline bool cmsg_copy_to_user(struct cmsghdr *__cmsg)
+{
+ return 0;
+}
+
static inline size_t msg_data_left(struct msghdr *msg)
{
return iov_iter_count(&msg->msg_iter);
@@ -396,6 +402,8 @@ struct timespec64;
struct __kernel_timespec;
struct old_timespec32;
+DECLARE_STATIC_KEY_FALSE(tx_copy_cmsg_to_user_key);
+
struct scm_timestamping_internal {
struct timespec64 ts[3];
};
diff --git a/include/net/sock.h b/include/net/sock.h
index cce23ac4d514..9c728287d21d 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1804,7 +1804,7 @@ static inline void sockcm_init(struct sockcm_cookie *sockc,
};
}
-int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
+int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
struct sockcm_cookie *sockc);
int sock_cmsg_send(struct sock *sk, struct msghdr *msg,
struct sockcm_cookie *sockc);
diff --git a/net/core/sock.c b/net/core/sock.c
index 9abc4fe25953..b2cbe753af1d 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2826,8 +2826,8 @@ struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
}
EXPORT_SYMBOL(sock_alloc_send_pskb);
-int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
- struct sockcm_cookie *sockc)
+int __sock_cmsg_send(struct sock *sk, struct msghdr *msg __always_unused,
+ struct cmsghdr *cmsg, struct sockcm_cookie *sockc)
{
u32 tsflags;
@@ -2881,7 +2881,7 @@ int sock_cmsg_send(struct sock *sk, struct msghdr *msg,
return -EINVAL;
if (cmsg->cmsg_level != SOL_SOCKET)
continue;
- ret = __sock_cmsg_send(sk, cmsg, sockc);
+ ret = __sock_cmsg_send(sk, msg, cmsg, sockc);
if (ret)
return ret;
}
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index cf377377b52d..6360b8ba9c84 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -267,7 +267,7 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc,
}
#endif
if (cmsg->cmsg_level == SOL_SOCKET) {
- err = __sock_cmsg_send(sk, cmsg, &ipc->sockc);
+ err = __sock_cmsg_send(sk, msg, cmsg, &ipc->sockc);
if (err)
return err;
continue;
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index fff78496803d..c9ae30acf895 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -777,7 +777,7 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
}
if (cmsg->cmsg_level == SOL_SOCKET) {
- err = __sock_cmsg_send(sk, cmsg, &ipc6->sockc);
+ err = __sock_cmsg_send(sk, msg, cmsg, &ipc6->sockc);
if (err)
return err;
continue;
diff --git a/net/socket.c b/net/socket.c
index fcbdd5bc47ac..4b65ac92045a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2537,8 +2537,49 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
return err < 0 ? err : 0;
}
-static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
- unsigned int flags, struct used_address *used_address,
+DEFINE_STATIC_KEY_FALSE(tx_copy_cmsg_to_user_key);
+
+static int sendmsg_copy_cmsg_to_user(struct msghdr *msg_sys,
+ struct user_msghdr __user *umsg)
+{
+ struct compat_msghdr __user *umsg_compat =
+ (struct compat_msghdr __user *)umsg;
+ unsigned int flags = msg_sys->msg_flags;
+ struct msghdr msg_user = *msg_sys;
+ unsigned long cmsg_ptr;
+ struct cmsghdr *cmsg;
+ int err;
+
+ msg_user.msg_control_is_user = true;
+ msg_user.msg_control_user = umsg->msg_control;
+ cmsg_ptr = (unsigned long)msg_user.msg_control;
+ for_each_cmsghdr(cmsg, msg_sys) {
+ if (!CMSG_OK(msg_sys, cmsg))
+ break;
+ if (!cmsg_copy_to_user(cmsg))
+ continue;
+ err = put_cmsg(&msg_user, cmsg->cmsg_level, cmsg->cmsg_type,
+ cmsg->cmsg_len - sizeof(*cmsg), CMSG_DATA(cmsg));
+ if (err)
+ return err;
+ }
+
+ err = __put_user((msg_sys->msg_flags & ~MSG_CMSG_COMPAT),
+ COMPAT_FLAGS(umsg));
+ if (err)
+ return err;
+ if (MSG_CMSG_COMPAT & flags)
+ err = __put_user((unsigned long)msg_user.msg_control - cmsg_ptr,
+ &umsg_compat->msg_controllen);
+ else
+ err = __put_user((unsigned long)msg_user.msg_control - cmsg_ptr,
+ &umsg->msg_controllen);
+ return err;
+}
+
+static int ____sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
+ struct msghdr *msg_sys, unsigned int flags,
+ struct used_address *used_address,
unsigned int allowed_msghdr_flags)
{
unsigned char ctl[sizeof(struct cmsghdr) + 20]
@@ -2549,6 +2590,8 @@ static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
ssize_t err;
err = -ENOBUFS;
+ if (static_branch_unlikely(&tx_copy_cmsg_to_user_key))
+ msg_sys->msg_control_copy_to_user = false;
if (msg_sys->msg_controllen > INT_MAX)
goto out;
@@ -2606,6 +2649,16 @@ static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
used_address->name_len);
}
+ if (static_branch_unlikely(&tx_copy_cmsg_to_user_key)) {
+ if (msg_sys->msg_control_copy_to_user && msg && err >= 0) {
+ ssize_t len = err;
+
+ err = sendmsg_copy_cmsg_to_user(msg_sys, msg);
+ if (!err)
+ err = len;
+ }
+ }
+
out_freectl:
if (ctl_buf != ctl)
sock_kfree_s(sock->sk, ctl_buf, ctl_len);
@@ -2648,8 +2701,8 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
if (err < 0)
return err;
- err = ____sys_sendmsg(sock, msg_sys, flags, used_address,
- allowed_msghdr_flags);
+ err = ____sys_sendmsg(sock, msg, msg_sys, flags, used_address,
+ allowed_msghdr_flags);
kfree(iov);
return err;
}
@@ -2660,7 +2713,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
long __sys_sendmsg_sock(struct socket *sock, struct msghdr *msg,
unsigned int flags)
{
- return ____sys_sendmsg(sock, msg, flags, NULL, 0);
+ return ____sys_sendmsg(sock, NULL, msg, flags, NULL, 0);
}
long __sys_sendmsg(int fd, struct user_msghdr __user *msg, unsigned int flags,
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v8 0/3] net: A lightweight zero-copy notification mechanism for MSG_ZEROCOPY
From: zijianzhang @ 2024-07-30 18:41 UTC (permalink / raw)
To: netdev
Cc: linux-api, willemdebruijn.kernel, almasrymina, edumazet, davem,
kuba, pabeni, dsahern, axboe, shuah, linux-kselftest, cong.wang,
xiaochun.lu, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
Original notification mechanism needs poll + recvmmsg which is not
easy for applcations to accommodate. And, it also incurs unignorable
overhead including extra system calls.
While making maximum reuse of the existing MSG_ZEROCOPY related code,
this patch set introduces a new zerocopy socket notification mechanism.
Users of sendmsg pass a control message as a placeholder for the incoming
notifications. Upon returning, kernel embeds notifications directly into
user arguments passed in. By doing so, we can reduce the complexity and
the overhead for managing notifications.
We also have the logic related to copying cmsg to the userspace in sendmsg
generic for any possible uses cases in the future. However, it introduces
ABI change of sendmsg.
Changelog:
v1 -> v2:
- Reuse errormsg queue in the new notification mechanism,
users can actually use these two mechanisms in hybrid way
if they want to do so.
- Update case SCM_ZC_NOTIFICATION in __sock_cmsg_send
1. Regardless of 32-bit, 64-bit program, we will always handle
u64 type user address.
2. The size of data to copy_to_user is precisely calculated
in case of kernel stack leak.
- fix (kbuild-bot)
1. Add SCM_ZC_NOTIFICATION to arch-specific header files.
2. header file types.h in include/uapi/linux/socket.h
v2 -> v3:
- 1. Users can now pass in the address of the zc_info_elem directly
with appropriate cmsg_len instead of the ugly user interface. Plus,
the handler is now compatible with MSG_CMSG_COMPAT and 32-bit
pointer.
- 2. Suggested by Willem, another strategy of getting zc info is
briefly taking the lock of sk_error_queue and move to a private
list, like net_rx_action. I thought sk_error_queue is protected by
sock_lock, so that it's impossible for the handling of zc info and
users recvmsg from the sk_error_queue at the same time.
However, sk_error_queue is protected by its own lock. I am afraid
that during the time it is handling the private list, users may
fail to get other error messages in the queue via recvmsg. Thus,
I don't implement the splice logic in this version. Any comments?
v3 -> v4:
- 1. Change SOCK_ZC_INFO_MAX to 64 to avoid large stack frame size.
- 2. Fix minor typos.
- 3. Change cfg_zerocopy from int to enum in msg_zerocopy.c
Initially, we expect users to pass the user address of the user array
as a data in cmsg, so that the kernel can copy_to_user to this address
directly.
As Willem commented,
> The main design issue with this series is this indirection, rather
> than passing the array of notifications as cmsg.
> This trick circumvents having to deal with compat issues and having to
> figure out copy_to_user in ____sys_sendmsg (as msg_control is an
> in-kernel copy).
> This is quite hacky, from an API design PoV.
> As is passing a pointer, but expecting msg_controllen to hold the
> length not of the pointer, but of the pointed to user buffer.
> I had also hoped for more significant savings. Especially with the
> higher syscall overhead due to meltdown and spectre mitigations vs
> when MSG_ZEROCOPY was introduced and I last tried this optimization.
We solve it by supporting put_cmsg to userspace in sendmsg path starting
from v5.
v4 -> v5:
- 1. Passing user address directly to kernel raises concerns about
ABI. In this version, we support put_cmsg to userspace in TX path
to solve this problem.
v5 -> v6:
- 1. Cleanly copy cmsg to user upon returning of ___sys_sendmsg
v6 -> v7:
- 1. Remove flag MSG_CMSG_COPY_TO_USER, use a member in msghdr instead
- 2. Pass msg to __sock_cmsg_send.
- 3. sendmsg_copy_cmsg_to_user should be put at the end of
____sys_sendmsg to make sure msg_sys->msg_control is a valid pointer.
- 4. Add struct zc_info to contain the array of zc_info_elem, so that
the kernel can update the zc_info->size. Another possible solution is
updating the cmsg_len directly, but it will break for_each_cmsghdr.
- 5. Update selftest to make cfg_notification_limit have the same
semantics in both methods for better comparison.
v7 -> v8:
- 1. Add a static_branch in ____sys_sendmsg to avoid overhead in the
hot path.
- 2. Add ZC_NOTIFICATION_MAX to limit the max size of zc_info->arr.
- 3. Minimize the code in SCM_ZC_NOTIFICATION handler by adding a local
sk_buff_head.
* Performance
We update selftests/net/msg_zerocopy.c to accommodate the new mechanism,
cfg_notification_limit has the same semantics for both methods. Test
results are as follows, we update skb_orphan_frags_rx to the same as
skb_orphan_frags to support zerocopy in the localhost test.
cfg_notification_limit = 1, both method get notifications after 1 calling
of sendmsg. In this case, the new method has around 17% cpu savings in TCP
and 23% cpu savings in UDP.
+----------------------+---------+---------+---------+---------+
| Test Type / Protocol | TCP v4 | TCP v6 | UDP v4 | UDP v6 |
+----------------------+---------+---------+---------+---------+
| ZCopy (MB) | 7523 | 7706 | 7489 | 7304 |
+----------------------+---------+---------+---------+---------+
| New ZCopy (MB) | 8834 | 8993 | 9053 | 9228 |
+----------------------+---------+---------+---------+---------+
| New ZCopy / ZCopy | 117.42% | 116.70% | 120.88% | 126.34% |
+----------------------+---------+---------+---------+---------+
cfg_notification_limit = 32, both get notifications after 32 calling of
sendmsg, which means more chances to coalesce notifications, and less
overhead of poll + recvmsg for the original method. In this case, the new
method has around 7% cpu savings in TCP and slightly better cpu usage in
UDP. In the env of selftest, notifications of TCP are more likely to be
out of order than UDP, it's easier to coalesce more notifications in UDP.
The original method can get one notification with range of 32 in a recvmsg
most of the time. In TCP, most notifications' range is around 2, so the
original method needs around 16 recvmsgs to get notified in one round.
That's the reason for the "New ZCopy / ZCopy" diff in TCP and UDP here.
+----------------------+---------+---------+---------+---------+
| Test Type / Protocol | TCP v4 | TCP v6 | UDP v4 | UDP v6 |
+----------------------+---------+---------+---------+---------+
| ZCopy (MB) | 8842 | 8735 | 10072 | 9380 |
+----------------------+---------+---------+---------+---------+
| New ZCopy (MB) | 9366 | 9477 | 10108 | 9385 |
+----------------------+---------+---------+---------+---------+
| New ZCopy / ZCopy | 106.00% | 108.28% | 100.31% | 100.01% |
+----------------------+---------+---------+---------+---------+
In conclusion, when notification interval is small or notifications are
hard to be coalesced, the new mechanism is highly recommended. Otherwise,
the performance gain from the new mechanism is very limited.
Zijian Zhang (3):
sock: support copying cmsgs to the user space in sendmsg
sock: add MSG_ZEROCOPY notification mechanism based on msg_control
selftests: add MSG_ZEROCOPY msg_control notification test
arch/alpha/include/uapi/asm/socket.h | 2 +
arch/mips/include/uapi/asm/socket.h | 2 +
arch/parisc/include/uapi/asm/socket.h | 2 +
arch/sparc/include/uapi/asm/socket.h | 2 +
include/linux/socket.h | 8 ++
include/net/sock.h | 2 +-
include/uapi/asm-generic/socket.h | 2 +
include/uapi/linux/socket.h | 23 +++++
net/core/sock.c | 72 +++++++++++++-
net/ipv4/ip_sockglue.c | 2 +-
net/ipv6/datagram.c | 2 +-
net/socket.c | 63 +++++++++++-
tools/testing/selftests/net/msg_zerocopy.c | 101 ++++++++++++++++++--
tools/testing/selftests/net/msg_zerocopy.sh | 1 +
14 files changed, 265 insertions(+), 19 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH 7/8] fs: add an ioctl to get the mnt ns id from nsfs
From: Dmitry V. Levin @ 2024-07-30 16:45 UTC (permalink / raw)
To: Josef Bacik; +Cc: Christian Brauner, linux-fsdevel, linux-api, kernel-team
In-Reply-To: <180449959d5a756af7306d6bda55f41b9d53e3cb.1719243756.git.josef@toxicpanda.com>
Hi,
On Mon, Jun 24, 2024 at 11:49:50AM -0400, Josef Bacik wrote:
> In order to utilize the listmount() and statmount() extensions that
> allow us to call them on different namespaces we need a way to get the
> mnt namespace id from user space. Add an ioctl to nsfs that will allow
> us to extract the mnt namespace id in order to make these new extensions
> usable.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> fs/nsfs.c | 14 ++++++++++++++
> include/uapi/linux/nsfs.h | 2 ++
> 2 files changed, 16 insertions(+)
>
> diff --git a/fs/nsfs.c b/fs/nsfs.c
> index 07e22a15ef02..af352dadffe1 100644
> --- a/fs/nsfs.c
> +++ b/fs/nsfs.c
> @@ -12,6 +12,7 @@
> #include <linux/nsfs.h>
> #include <linux/uaccess.h>
>
> +#include "mount.h"
> #include "internal.h"
>
> static struct vfsmount *nsfs_mnt;
> @@ -143,6 +144,19 @@ static long ns_ioctl(struct file *filp, unsigned int ioctl,
> argp = (uid_t __user *) arg;
> uid = from_kuid_munged(current_user_ns(), user_ns->owner);
> return put_user(uid, argp);
> + case NS_GET_MNTNS_ID: {
> + struct mnt_namespace *mnt_ns;
> + __u64 __user *idp;
> + __u64 id;
> +
> + if (ns->ops->type != CLONE_NEWNS)
> + return -EINVAL;
> +
> + mnt_ns = container_of(ns, struct mnt_namespace, ns);
> + idp = (__u64 __user *)arg;
> + id = mnt_ns->seq;
> + return put_user(id, idp);
> + }
> default:
> return -ENOTTY;
> }
> diff --git a/include/uapi/linux/nsfs.h b/include/uapi/linux/nsfs.h
> index a0c8552b64ee..56e8b1639b98 100644
> --- a/include/uapi/linux/nsfs.h
> +++ b/include/uapi/linux/nsfs.h
> @@ -15,5 +15,7 @@
> #define NS_GET_NSTYPE _IO(NSIO, 0x3)
> /* Get owner UID (in the caller's user namespace) for a user namespace */
> #define NS_GET_OWNER_UID _IO(NSIO, 0x4)
> +/* Get the id for a mount namespace */
> +#define NS_GET_MNTNS_ID _IO(NSIO, 0x5)
As the kernel is writing an object of type __u64,
this has to be defined to _IOR(NSIO, 0x5, __u64) instead,
see the corresponding comments in uapi/asm-generic/ioctl.h file.
--
ldv
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Christoph Hellwig @ 2024-07-30 15:38 UTC (permalink / raw)
To: Dave Chinner
Cc: Theodore Ts'o, Mateusz Guzik, Florian Weimer, linux-fsdevel,
linux-kernel, linux-api, Dave Chinner
In-Reply-To: <ZqhQnWQSweXgffdD@dread.disaster.area>
On Tue, Jul 30, 2024 at 12:31:57PM +1000, Dave Chinner wrote:
> There are at least two different "is this inode identical"
> use cases that {st_dev,st_ino} is being used for.
>
> The first, as Florian described, is to determine if two open fds
> refer to the same inode for collision avoidance.
>
> This works on traditional filesystems like ext4 and XFS, but isn't
> reliable on filesystems with integrated snapshot/subvolume
> functionality.
It's not about snapshot, it's about file systems being broken. Even
btrfs for example always has a unique st_dev,st_ino pair, it can
just unexpectly change at any subvolume root and not just at a mount
point.
> That is our long term challenge: replacing the use of {dev,ino} for
> data uniqueness disambiguation. Making the identification of owners
> of non-unique/shared data simple for applications to use and fast
> for filesystems to resolve will be a challenge.
I don't think there is any way to provide such a guarantee as there
is so many levels of cloning or dedup, many of which are totally
invisible to the high level file system interface.
^ permalink raw reply
* [PATCH v3] epoll.7: clarify the event distribution under edge-triggered mode
From: Andy Pan via B4 Relay @ 2024-07-30 10:48 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: linux-man, linux-api, Andy Pan
From: Andy Pan <i@andypan.me>
For the moment, the edge-triggered epoll generates an event for each
receipt of a chunk of data, that is to say, epoll_wait() will return
and tell us a monitored file descriptor is ready whenever there is a
new activity on that FD since we were last informed about that FD.
This is not a real _edge_ implementation for epoll, but it's been
working this way for years and plenty of projects are relying on it
to eliminate the overhead of one system call of read(2) per wakeup event.
There are several renowned open-source projects relying on this feature
for notification function (with eventfd): register eventfd with EPOLLET
and avoid calling read(2) on the eventfd when there is wakeup event (eventfd being written).
Examples: nginx [1], netty [2], tokio [3], libevent [4], ect. [5]
These projects are widely used in today's Internet infrastructures.
Thus, changing this behavior of epoll ET will fundamentally break them
and cause a significant negative impact.
Linux has changed it for pipe before [6], breaking some Android libraries,
which had got "reverted" somehow. [7] [8]
Nevertheless, the paragraph in the manual pages describing this
characteristic of epoll ET seems ambiguous, I think a more explict
sentence should be used to clarify it. We're improving the notification
mechanism for libuv recently by exploiting this feature with eventfd,
which brings us a significant performance boost. [9]
Therefore, we (as well as the maintainers of nginx, netty, tokio, etc.)
would have a sense of security to build an enhanced notification function
based on this feature if there is a guarantee of retaining this implementation
of epoll ET for the backward compatibility in the man pages.
[1]: https://github.com/nginx/nginx/blob/efc6a217b92985a1ee211b6bb7337cd2f62deb90/src/event/modules/ngx_epoll_module.c#L386-L457
[2]: https://github.com/netty/netty/pull/9192
[3]: https://github.com/tokio-rs/mio/blob/309daae21ecb1d46203a7dbc0cf4c80310240cba/src/sys/unix/waker.rs#L111-L143
[4]: https://github.com/libevent/libevent/blob/525f5d0a14c9c103be750f2ca175328c25505ea4/event.c#L2597-L2614
[5]: https://github.com/libuv/libuv/pull/4400#issuecomment-2123798748
[6]: https://lkml.iu.edu/hypermail/linux/kernel/2010.1/04363.html
[7]: https://github.com/torvalds/linux/commit/3a34b13a88caeb2800ab44a4918f230041b37dd9
[8]: https://github.com/torvalds/linux/commit/3b844826b6c6affa80755254da322b017358a2f4
[9]: https://github.com/libuv/libuv/pull/4400#issuecomment-2103232402
Signed-off-by: Andy Pan <i@andypan.me>
---
Changes in v3:
- Updated the git commit description
- Link to v2: https://lore.kernel.org/r/20240727-epoll-et-desc-v2-1-c99b2ac66775@andypan.me
Changes in v2:
- Added the git commit description based on feedback
- Link to v1: https://lore.kernel.org/r/20240727-epoll-et-desc-v1-1-390bafc678b9@andypan.me
---
man/man7/epoll.7 | 1 +
1 file changed, 1 insertion(+)
diff --git a/man/man7/epoll.7 b/man/man7/epoll.7
index 951500131..361d9db99 100644
--- a/man/man7/epoll.7
+++ b/man/man7/epoll.7
@@ -172,6 +172,7 @@ .SS Level-triggered and edge-triggered
Since even with edge-triggered
.BR epoll ,
multiple events can be generated upon receipt of multiple chunks of data,
+that is, an event will be generated upon each receipt of a chunk of data,
the caller has the option to specify the
.B EPOLLONESHOT
flag, to tell
---
base-commit: cbc0a111e4dceea2037c51098de33e6bc8c16a5c
change-id: 20240727-epoll-et-desc-04ea9a590f3b
Best regards,
--
Andy Pan <i@andypan.me>
^ permalink raw reply related
* Re: Testing if two open descriptors refer to the same inode
From: Theodore Ts'o @ 2024-07-30 4:19 UTC (permalink / raw)
To: Dave Chinner
Cc: Mateusz Guzik, Florian Weimer, linux-fsdevel, linux-kernel,
linux-api, Dave Chinner
In-Reply-To: <ZqhQnWQSweXgffdD@dread.disaster.area>
On Tue, Jul 30, 2024 at 12:31:57PM +1000, Dave Chinner wrote:
> I don't think you can make such a simplistic delineation, because
> there's more than one issue at play here.
>
> There are at least two different "is this inode identical"
> use cases that {st_dev,st_ino} is being used for.
>
> The first, as Florian described, is to determine if two open fds
> refer to the same inode for collision avoidance.
>
> This works on traditional filesystems like ext4 and XFS, but isn't
> reliable on filesystems with integrated snapshot/subvolume
> functionality.
That's fair, but the first is the problem I think is more important,
because there are existing programs which are depending on
st_dev/st_ino as being a reliable way of detecting uniqueness --- and
if this breas, file data may get corrpted, or the dyanmic linker will
assume that two unrelated shared libraries are actually the same, with
hilarity then ensuing --- because an interface which is guaranteed by
decade of Unix history, and POSIX, is broken by some Linux file
systems.
> The second is that {dev,ino} is being used to disambiguate paths
> that point to hardlinked inodes for the purposes of identifying
> and optimising access and replication of shared (i.e. non-unique)
> file data.
>
> This works on traditional filesystems like ext4, but fails badly on
> filesystem that support FICLONERANGE (XFS, btrfs, NFS, CIFS,
> bcachefs, etc) because cloned files have unique inodes but
> non-unique data.
That's a problem, yes --- but it's a previously unsolved problem, and
the failure will cause inefficiency, but it doesn't actually cause
data corruption.
It's also a very hard problem, especially if we're considering the
full, general FICLONERANGE interface, where an arbitrary subset of
blocks at one offset, might be cloned at a completely different offset
in a different file, and where a single file might have cloned ranges
from a dozens of other files. How this information would be
communicated to userspace, so they could copy a directory hierarchy
without an increased expansion is a hard problem.
Given that we have a simple solution (filehandles) to fix a problem
where false positives causes lost data or core dumps, let's solve the
simple problem and try to get it standardized acrossed operating
systems beyond Linux, and in parallel, we can try to figure out a much
more complicated interface to solve this other problem.
- Ted
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Dave Chinner @ 2024-07-30 2:31 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Mateusz Guzik, Florian Weimer, linux-fsdevel, linux-kernel,
linux-api, Dave Chinner
In-Reply-To: <20240729133601.GA557749@mit.edu>
On Mon, Jul 29, 2024 at 09:36:01AM -0400, Theodore Ts'o wrote:
> On Mon, Jul 29, 2024 at 12:18:15PM +0200, Mateusz Guzik wrote:
> >
> > Are you claiming on-disk inode numbers are not guaranteed unique per
> > filesystem? It sounds like utter breakage, with capital 'f'.
>
> The reality is that there exists file systems which do not return
> unique inode numbers. For example, there are virtiofs implementations
> which pass the inode numbers straight through with a fixed dev_t. If
> you have a large number of packages mounted via iscsi, and those
> packages include shared libraries, then you can have two different
> shared libraries with the same inode number, and then you can watch
> the dynamic liunker get Very Confused, and debugging the problem can
> be.... interesting. (Three gueses how I found out about this, and the
> first two don't count. Yes, we figured out a workaround.)
>
> So that breakage exists already, today.
>
> For people who don't like this, they can stick to those file systems
> that still guarantee unique inode numbers, at least for local disk
> file systems --- for example, to use ext4 and xfs, over btrfs and
> bcachefs.
I don't think you can make such a simplistic delineation, because
there's more than one issue at play here.
There are at least two different "is this inode identical"
use cases that {st_dev,st_ino} is being used for.
The first, as Florian described, is to determine if two open fds
refer to the same inode for collision avoidance.
This works on traditional filesystems like ext4 and XFS, but isn't
reliable on filesystems with integrated snapshot/subvolume
functionality.
The second is that {dev,ino} is being used to disambiguate paths
that point to hardlinked inodes for the purposes of identifying
and optimising access and replication of shared (i.e. non-unique)
file data.
This works on traditional filesystems like ext4, but fails badly on
filesystem that support FICLONERANGE (XFS, btrfs, NFS, CIFS,
bcachefs, etc) because cloned files have unique inodes but
non-unique data.
> However, this is a short-term expedient, and in the long term, we will
> need to guide userspace to use something that is more likely to work,
> such as file handles.
The first case can be done with filehandles - it's a simple
resolution of fds to filehandles and compare the opaque filehandles.
That's the short-term solution because it's the easy one to solve.
However, filehandles do not work for the solving the second case.
Hardlinks are basically a mechanism for avoiding data copying within
the same filesystem. i.e. hardlink disambiguation is essentially
the process of detecting which dirents point to the same shared
data. We can detect a hardlinked inode simply by looking at the
link count, and we use the inode number to determine that they point
to the same physical storage.
Applications like tar and rsync are detecting hard links to avoid
two main issues:
- moving the same data multiple times
- causing destination write amplification by storing the
same data in multiple places
They avoid these by creating a hardlink map of the directory
structure being processed, and then recreate that hardlink map at
the destination. We could use filehandles for that, too, and then
we wouldn't be relying on {dev,ino} for this, either.
However, any application that is using inode number or filehandle
comparisons to detect data uniqueness does not work if other
applications and utilities are using reflink copies rather than
hardlinks for space efficient data copying.
Let's all keep this in mind here: the default behaviour of `cp` is
to use file clones on filesystems that support them over physical
data copies. I have maybe half a dozen hardlinks in most of my local
XFS filesystems, but I have many tens of thousands of cloned files
in those same filesystems.
IOWs, any tool that is using {dev,ino} as a proxy for data
uniqueness is fundamentally deficient on any filesystem that
supports file cloning.
Given the potential for badness in replicating filesystems full of
cloned data, it's far more important and higher priority for such
utilities to move away from using {dev,ino} to detect data
uniqueness. Handling cloned data efficiently requires this, and
that's a far better reason for moving away from {dev,ino} based
disambiguation than "oh, it doesn't work on btrfs properly".
Detecting "is the data unique" is not that hard - e.g. we could
add a statx flag to say "this inode has shared data" - and then
userspace can tag that inode as needing data disambiguation before
starting to move data.
However, data disambiguation (i.e. finding what inodes share the
data at which file offset) is a much harder problem. This largely
requires knowledge of the entire layout of the filesystem, and so
it's really only a question the filesystem itself can resolve.
We already provide such an interface for XFS with ioctl(GETFSMAP).
It is driven by the on-disk reverse mapping btrees, and so can
quickly answer the "what {inode,offset,len} tuples share this
physical extent" question. The interface is generic, however, so
asking such a question and determining the answer is .... complex.
That is our long term challenge: replacing the use of {dev,ino} for
data uniqueness disambiguation. Making the identification of owners
of non-unique/shared data simple for applications to use and fast
for filesystems to resolve will be a challenge.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Dave Chinner @ 2024-07-29 23:19 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Aleksa Sarai, Florian Weimer, linux-fsdevel, linux-kernel,
linux-api, Dave Chinner, Christian Brauner
In-Reply-To: <CAGudoHFQ9TtG-5__38-ND4KTxYCpEKVv_X9HhZixcdnVMUBEwQ@mail.gmail.com>
On Mon, Jul 29, 2024 at 02:12:15PM +0200, Mateusz Guzik wrote:
> There needs to be a dev + ino replacement which retains the property
> of being reliable between reboots and so on.
Yes, that's exactly what filehandles provide.
OTOH, dev + ino by itself does not provide this guarantee, as st_dev
is not guaranteed to be constant across reboots because of things
like async device discovery and enumeration. i.e. userspace has to
do a bunch of work to determine the "st_dev" for a given filesystem
actually refers to the same filesystem it did before the system
restarted (e.g. via FS uuid checks).
Filehandles already contain persistent filesystem identifiers, and
so userspace doesn't have to do anything special to ensure that
comparisons across reboots are valid.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Dave Chinner @ 2024-07-29 23:08 UTC (permalink / raw)
To: Florian Weimer
Cc: Mateusz Guzik, linux-fsdevel, linux-kernel, linux-api,
Dave Chinner
In-Reply-To: <87sevspit1.fsf@oldenburg.str.redhat.com>
On Mon, Jul 29, 2024 at 12:57:14PM +0200, Florian Weimer wrote:
> * Mateusz Guzik:
>
> > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> >> * Mateusz Guzik:
> >>
> >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> >> >> It was pointed out to me that inode numbers on Linux are no longer
> >> >> expected to be unique per file system, even for local file systems.
> >> >
> >> > I don't know if I'm parsing this correctly.
> >> >
> >> > Are you claiming on-disk inode numbers are not guaranteed unique per
> >> > filesystem? It sounds like utter breakage, with capital 'f'.
> >>
> >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> >> local file systems are different.
> >
> > Can you link me some threads about this?
>
> Sorry, it was an internal thread. It's supposed to be common knowledge
> among Linux file system developers.
btrfs has been dealing with this issue since snapshots/subvols were
first introduced some 15-odd years ago. This isn't a new problem...
https://lore.kernel.org/linux-fsdevel/20231025210654.GA2892534@perftesting/
> Aleksa referenced LSF/MM
> discussions.
I also referenced those discussions and the -fsdevel discussions
that have been happening for quite some time. I'll reference some
of them here again, because they are all out in the open....
https://lwn.net/Articles/975444/
https://lore.kernel.org/linux-fsdevel/?q=st_vol
https://lore.kernel.org/linux-fsdevel/20231211233231.oiazgkqs7yahruuw@moria.home.lan/
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Florian Weimer @ 2024-07-29 15:39 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner
In-Reply-To: <a13bff5812cb36adf3fed80093cbe1de601ec506.camel@kernel.org>
* Jeff Layton:
> On Mon, 2024-07-29 at 08:55 +0200, Florian Weimer wrote:
>> It was pointed out to me that inode numbers on Linux are no longer
>> expected to be unique per file system, even for local file systems.
>> Applications sometimes need to check if two (open) files are the
>> same.
>> For example, a program may want to use a temporary file if is invoked
>> with input and output files referring to the same file.
>>
>> How can we check for this? The POSIX way is to compare st_ino and
>> st_dev in stat output, but if inode numbers are not unique, that will
>> result in files falsely being reported as identical. It's harmless
>> in
>> the temporary file case, but it in other scenarios, it may result in
>> data loss.
>>
>
> I believe this is the problem that STATX_SUBVOL was intended to solve.
>
> Both bcachefs and btrfs will provide this attribute if requested. So,
> basically to uniquely ID an inode using statx, you need a tuple of:
>
> stx_dev_major/minor
> stx_subvol
> stx_ino
>
> If the filesystem doesn't provide STATX_SUBVOL, then one can (likely)
> conclude that stx_dev_* and stx_ino are enough.
Does this really work for the virtiofs case, though? It has to pass
through all three *and* make things unique relative to the host, I
think.
Thanks,
Florian
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Jeff Layton @ 2024-07-29 15:24 UTC (permalink / raw)
To: Florian Weimer, linux-fsdevel, linux-kernel, linux-api; +Cc: Dave Chinner
In-Reply-To: <874j88sn4d.fsf@oldenburg.str.redhat.com>
On Mon, 2024-07-29 at 08:55 +0200, Florian Weimer wrote:
> It was pointed out to me that inode numbers on Linux are no longer
> expected to be unique per file system, even for local file systems.
> Applications sometimes need to check if two (open) files are the
> same.
> For example, a program may want to use a temporary file if is invoked
> with input and output files referring to the same file.
>
> How can we check for this? The POSIX way is to compare st_ino and
> st_dev in stat output, but if inode numbers are not unique, that will
> result in files falsely being reported as identical. It's harmless
> in
> the temporary file case, but it in other scenarios, it may result in
> data loss.
>
I believe this is the problem that STATX_SUBVOL was intended to solve.
Both bcachefs and btrfs will provide this attribute if requested. So,
basically to uniquely ID an inode using statx, you need a tuple of:
stx_dev_major/minor
stx_subvol
stx_ino
If the filesystem doesn't provide STATX_SUBVOL, then one can (likely)
conclude that stx_dev_* and stx_ino are enough.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Theodore Ts'o @ 2024-07-29 13:36 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Florian Weimer, linux-fsdevel, linux-kernel, linux-api,
Dave Chinner
In-Reply-To: <ghqndyn4x7ujxvybbwet5vxiahus4zey6nkfsv6he3d4en6ehu@bq5s23lstzor>
On Mon, Jul 29, 2024 at 12:18:15PM +0200, Mateusz Guzik wrote:
>
> Are you claiming on-disk inode numbers are not guaranteed unique per
> filesystem? It sounds like utter breakage, with capital 'f'.
The reality is that there exists file systems which do not return
unique inode numbers. For example, there are virtiofs implementations
which pass the inode numbers straight through with a fixed dev_t. If
you have a large number of packages mounted via iscsi, and those
packages include shared libraries, then you can have two different
shared libraries with the same inode number, and then you can watch
the dynamic liunker get Very Confused, and debugging the problem can
be.... interesting. (Three gueses how I found out about this, and the
first two don't count. Yes, we figured out a workaround.)
So that breakage exists already, today.
For people who don't like this, they can stick to those file systems
that still guarantee unique inode numbers, at least for local disk
file systems --- for example, to use ext4 and xfs, over btrfs and
bcachefs.
However, this is a short-term expedient, and in the long term, we will
need to guide userspace to use something that is more likely to work,
such as file handles. And ideally, this needs to be standardized at
venues such as the Austin Group, so that it becomes interfaces which
are used across operating systems, not just for Linux. It's going to
be a multi-year, if not decade-long, effort...
- Ted
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Christian Brauner @ 2024-07-29 12:26 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Florian Weimer, linux-fsdevel, linux-kernel, linux-api,
Dave Chinner
In-Reply-To: <ghqndyn4x7ujxvybbwet5vxiahus4zey6nkfsv6he3d4en6ehu@bq5s23lstzor>
On Mon, Jul 29, 2024 at 12:18:15PM GMT, Mateusz Guzik wrote:
> On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> > It was pointed out to me that inode numbers on Linux are no longer
> > expected to be unique per file system, even for local file systems.
>
> I don't know if I'm parsing this correctly.
>
> Are you claiming on-disk inode numbers are not guaranteed unique per
> filesystem? It sounds like utter breakage, with capital 'f'.
>
> I know the 32-bit inode allocation code can result in unintentional
> duplicates after wrap around (see get_next_ino), but that's for
> in-memory stuff only(?) like pipes, so perhaps tolerable.
>
> Anyhow, the kernel recently got F_DUPFD_QUERY which tests if the *file*
> object is the same.
>
> While the above is not what's needed here, I guess it sets a precedent
> for F_DUPINODE_QUERY (or whatever other name) to be added to handily
> compare inode pointers. It may be worthwhile regardless of the above.
> (or maybe kcmp could be extended?)
We don't use kcmp() for such core operations. It's overkill and security
sensitive so quite a few configs don't enable it. See [1] for details
how kcmp() can be used to facilitate UAF attacks and defeat
probabilistic UAF mitigations by using ordering information. So that
ordering requirement should really go out the window.
Another thing is that kcmp() works cross-process which is also out of
scope for this. kcmp() really should be reserved for stuff that
checkpoint/restore in userspace needs and that's otherwise too fruity to
be implemented in our regular apis.
[1]: https://googleprojectzero.blogspot.com/2021/10/how-simple-linux-kernel-memory.html
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Mateusz Guzik @ 2024-07-29 12:12 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Florian Weimer, linux-fsdevel, linux-kernel, linux-api,
Dave Chinner, Christian Brauner
In-Reply-To: <20240729.114221-bumpy.fronds.spare.forts-a2tVepJTDtVb@cyphar.com>
On Mon, Jul 29, 2024 at 1:47 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2024-07-29, Mateusz Guzik <mjguzik@gmail.com> wrote:
> > On Mon, Jul 29, 2024 at 12:57 PM Florian Weimer <fweimer@redhat.com> wrote:
> > >
> > > * Mateusz Guzik:
> > >
> > > > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> > > >> * Mateusz Guzik:
> > > >>
> > > >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> > > >> >> It was pointed out to me that inode numbers on Linux are no longer
> > > >> >> expected to be unique per file system, even for local file systems.
> > > >> >
> > > >> > I don't know if I'm parsing this correctly.
> > > >> >
> > > >> > Are you claiming on-disk inode numbers are not guaranteed unique per
> > > >> > filesystem? It sounds like utter breakage, with capital 'f'.
> > > >>
> > > >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> > > >> local file systems are different.
> > > >
> > > > Can you link me some threads about this?
> > >
> > > Sorry, it was an internal thread. It's supposed to be common knowledge
> > > among Linux file system developers. Aleksa referenced LSF/MM
> > > discussions.
> > >
> >
> > So much for open development :-P
> >
> > > > I had this in mind (untested modulo compilation):
> > > >
> > > > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > > > index 300e5d9ad913..5723c3e82eac 100644
> > > > --- a/fs/fcntl.c
> > > > +++ b/fs/fcntl.c
> > > > @@ -343,6 +343,13 @@ static long f_dupfd_query(int fd, struct file *filp)
> > > > return f.file == filp;
> > > > }
> > > >
> > > > +static long f_dupfd_query_inode(int fd, struct file *filp)
> > > > +{
> > > > + CLASS(fd_raw, f)(fd);
> > > > +
> > > > + return f.file->f_inode == filp->f_inode;
> > > > +}
> > > > +
> > > > static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > > > struct file *filp)
> > > > {
> > > > @@ -361,6 +368,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > > > case F_DUPFD_QUERY:
> > > > err = f_dupfd_query(argi, filp);
> > > > break;
> > > > + case F_DUPFD_QUERY_INODE:
> > > > + err = f_dupfd_query_inode(argi, filp);
> > > > + break;
> > > > case F_GETFD:
> > > > err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
> > > > break;
> > > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > > index c0bcc185fa48..2e93dbdd8fd2 100644
> > > > --- a/include/uapi/linux/fcntl.h
> > > > +++ b/include/uapi/linux/fcntl.h
> > > > @@ -16,6 +16,8 @@
> > > >
> > > > #define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
> > > >
> > > > +#define F_DUPFD_QUERY_INODE (F_LINUX_SPECIFIC_BASE + 4)
> > > > +
> > > > /*
> > > > * Cancel a blocking posix lock; internal use only until we expose an
> > > > * asynchronous lock api to userspace:
> > >
> > > It's certainly much easier to use than name_to_handle_at, so it looks
> > > like a useful option to have.
> > >
> > > Could we return a three-way comparison result for sorting? Or would
> > > that expose too much about kernel pointer values?
> > >
> >
> > As is this would sort by inode *address* which I don't believe is of
> > any use -- the order has to be assumed arbitrary.
> >
> > Perhaps there is something which is reliably the same and can be
> > combined with something else to be unique system-wide (the magic
> > handle thing?).
> >
> > But even then you would need to justify trying to sort by fcntl calls,
> > which sounds pretty dodgey to me.
>
> Programs need to key things by (dev, ino) currently, so you need to be
> able to get some kind of ordinal that you can sort with.
>
That I know, except normally that's done by sorting by (f)stat results.
> If we really want to make an interface to let you do this without
> exposing hashes in statx, then kcmp(2) makes more sense, but having to
> keep a file descriptor for each entry in a hashtable would obviously
> cause -EMFILE issues.
>
Agreed, hence the proposal to extend statx.
> > Given that thing I *suspect* statx() may want to get extended with
> > some guaranteed unique identifier. Then you can sort in userspace all
> > you want.
>
> Yeah, this is what the hashed fhandle patch I have does.
>
Ok, I see your other e-mail.
> > Based on your opening mail I assumed you only need to check 2 files,
> > for which the proposed fcntl does the trick.
> >
> > Or to put it differently: there seems to be more to the picture than
> > in the opening mail, so perhaps you could outline what you are looking
> > for.
>
> Hardlink detection requires creating a hashmap of (dev, ino) to find
> hardlinks. Pair-wise checking is not sufficient for that usecase (which
> AFAIK is the most common thing people use inode numbers for -- it's at
> least probably the most common thing people run in practice since
> archive tools do this.)
>
So if you have *numerous* files to check then indeed the fcntl is no
good, but the sorting variant is no good either -- you don't know what
key to look stuff up by since you don't know any of the addresses
(obfuscated or otherwise).
There needs to be a dev + ino replacement which retains the property
of being reliable between reboots and so on.
Since you said you have a patchset which exports something in statx,
chances are this is sorted out -- I'm gong to wait for that, meanwhile
I'm not going to submit my fcntl anywhere -- hopefuly it will be
avoided. :)
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Mateusz Guzik @ 2024-07-29 12:00 UTC (permalink / raw)
To: Florian Weimer
Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner,
Christian Brauner
In-Reply-To: <87cymwpgys.fsf@oldenburg.str.redhat.com>
On Mon, Jul 29, 2024 at 1:37 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Mateusz Guzik:
>
> > On Mon, Jul 29, 2024 at 12:57 PM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Mateusz Guzik:
> >>
> >> > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> >> >> * Mateusz Guzik:
> >> >>
> >> >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> >> >> >> It was pointed out to me that inode numbers on Linux are no longer
> >> >> >> expected to be unique per file system, even for local file systems.
> >> >> >
> >> >> > I don't know if I'm parsing this correctly.
> >> >> >
> >> >> > Are you claiming on-disk inode numbers are not guaranteed unique per
> >> >> > filesystem? It sounds like utter breakage, with capital 'f'.
> >> >>
> >> >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> >> >> local file systems are different.
> >> >
> >> > Can you link me some threads about this?
> >>
> >> Sorry, it was an internal thread. It's supposed to be common knowledge
> >> among Linux file system developers. Aleksa referenced LSF/MM
> >> discussions.
> >>
> >
> > So much for open development :-P
>
> I found this pretty quickly, so it does seem widely known:
>
> [LSF TOPIC] statx extensions for subvol/snapshot filesystems & more
> <https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/>
>
Huh, thanks.
> >> It's certainly much easier to use than name_to_handle_at, so it looks
> >> like a useful option to have.
> >>
> >> Could we return a three-way comparison result for sorting? Or would
> >> that expose too much about kernel pointer values?
> >>
> >
> > As is this would sort by inode *address* which I don't believe is of
> > any use -- the order has to be assumed arbitrary.
>
> Doesn't the order remain valid while the files remain open? Anything
> else doesn't seem reasonable to expect anyway.
>
They will indeed remain stable in that setting, I am saying ordering
may be different after a reboot or if there was some memory
reclamation going on between restarts of the program.
This is quite a difference from dev + ino combo not suffering these problems.
That is to say I don't see what is the benefit of having the kernel
provide a way to sort inodes in a way which can give different
results.
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Aleksa Sarai @ 2024-07-29 11:47 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Florian Weimer, linux-fsdevel, linux-kernel, linux-api,
Dave Chinner, Christian Brauner
In-Reply-To: <CAGudoHEBNRE+78n=WEY=Z0ZCnLmDFadisR-K2ah4SUO6uSm4TA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4688 bytes --]
On 2024-07-29, Mateusz Guzik <mjguzik@gmail.com> wrote:
> On Mon, Jul 29, 2024 at 12:57 PM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Mateusz Guzik:
> >
> > > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> > >> * Mateusz Guzik:
> > >>
> > >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> > >> >> It was pointed out to me that inode numbers on Linux are no longer
> > >> >> expected to be unique per file system, even for local file systems.
> > >> >
> > >> > I don't know if I'm parsing this correctly.
> > >> >
> > >> > Are you claiming on-disk inode numbers are not guaranteed unique per
> > >> > filesystem? It sounds like utter breakage, with capital 'f'.
> > >>
> > >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> > >> local file systems are different.
> > >
> > > Can you link me some threads about this?
> >
> > Sorry, it was an internal thread. It's supposed to be common knowledge
> > among Linux file system developers. Aleksa referenced LSF/MM
> > discussions.
> >
>
> So much for open development :-P
>
> > > I had this in mind (untested modulo compilation):
> > >
> > > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > > index 300e5d9ad913..5723c3e82eac 100644
> > > --- a/fs/fcntl.c
> > > +++ b/fs/fcntl.c
> > > @@ -343,6 +343,13 @@ static long f_dupfd_query(int fd, struct file *filp)
> > > return f.file == filp;
> > > }
> > >
> > > +static long f_dupfd_query_inode(int fd, struct file *filp)
> > > +{
> > > + CLASS(fd_raw, f)(fd);
> > > +
> > > + return f.file->f_inode == filp->f_inode;
> > > +}
> > > +
> > > static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > > struct file *filp)
> > > {
> > > @@ -361,6 +368,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > > case F_DUPFD_QUERY:
> > > err = f_dupfd_query(argi, filp);
> > > break;
> > > + case F_DUPFD_QUERY_INODE:
> > > + err = f_dupfd_query_inode(argi, filp);
> > > + break;
> > > case F_GETFD:
> > > err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
> > > break;
> > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > index c0bcc185fa48..2e93dbdd8fd2 100644
> > > --- a/include/uapi/linux/fcntl.h
> > > +++ b/include/uapi/linux/fcntl.h
> > > @@ -16,6 +16,8 @@
> > >
> > > #define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
> > >
> > > +#define F_DUPFD_QUERY_INODE (F_LINUX_SPECIFIC_BASE + 4)
> > > +
> > > /*
> > > * Cancel a blocking posix lock; internal use only until we expose an
> > > * asynchronous lock api to userspace:
> >
> > It's certainly much easier to use than name_to_handle_at, so it looks
> > like a useful option to have.
> >
> > Could we return a three-way comparison result for sorting? Or would
> > that expose too much about kernel pointer values?
> >
>
> As is this would sort by inode *address* which I don't believe is of
> any use -- the order has to be assumed arbitrary.
>
> Perhaps there is something which is reliably the same and can be
> combined with something else to be unique system-wide (the magic
> handle thing?).
>
> But even then you would need to justify trying to sort by fcntl calls,
> which sounds pretty dodgey to me.
Programs need to key things by (dev, ino) currently, so you need to be
able to get some kind of ordinal that you can sort with.
If we really want to make an interface to let you do this without
exposing hashes in statx, then kcmp(2) makes more sense, but having to
keep a file descriptor for each entry in a hashtable would obviously
cause -EMFILE issues.
> Given that thing I *suspect* statx() may want to get extended with
> some guaranteed unique identifier. Then you can sort in userspace all
> you want.
Yeah, this is what the hashed fhandle patch I have does.
> Based on your opening mail I assumed you only need to check 2 files,
> for which the proposed fcntl does the trick.
>
> Or to put it differently: there seems to be more to the picture than
> in the opening mail, so perhaps you could outline what you are looking
> for.
Hardlink detection requires creating a hashmap of (dev, ino) to find
hardlinks. Pair-wise checking is not sufficient for that usecase (which
AFAIK is the most common thing people use inode numbers for -- it's at
least probably the most common thing people run in practice since
archive tools do this.)
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Aleksa Sarai @ 2024-07-29 11:40 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Florian Weimer, linux-fsdevel, linux-kernel, linux-api,
Dave Chinner, Christian Brauner
In-Reply-To: <CAGudoHEBNRE+78n=WEY=Z0ZCnLmDFadisR-K2ah4SUO6uSm4TA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]
On 2024-07-29, Mateusz Guzik <mjguzik@gmail.com> wrote:
> On Mon, Jul 29, 2024 at 12:57 PM Florian Weimer <fweimer@redhat.com> wrote:
> > > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> > >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> > >> >> It was pointed out to me that inode numbers on Linux are no longer
> > >> >> expected to be unique per file system, even for local file systems.
> > >> >
> > >> > I don't know if I'm parsing this correctly.
> > >> >
> > >> > Are you claiming on-disk inode numbers are not guaranteed unique per
> > >> > filesystem? It sounds like utter breakage, with capital 'f'.
> > >>
> > >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> > >> local file systems are different.
> > >
> > > Can you link me some threads about this?
> >
> > Sorry, it was an internal thread. It's supposed to be common knowledge
> > among Linux file system developers. Aleksa referenced LSF/MM
> > discussions.
>
> So much for open development :-P
To be clear, this wasn't _decided_ at LSF/MM, it was brought up as a
topic. There is an LWN article about the session that mentions the
issue[1].
My understanding is that the btrfs and bcachefs folks independently
determined they cannot provide this guarantee. As far as I understand,
the reason why is that inode number allocation on btree filesystems
stores information about location and some other bits (maybe subvolumes)
in the bits, making it harder to guarantee there will be no collisions.
Don't quote me on that though, I'm sure they'll tell me I'm wrong when
they wake up. :D
As the article mentions, Kent Overstreet suggested trying to see how
many things break if you build a kernel where all inode numbers are the
same (my guess would be "very badly" -- aside from the classic problem
of hardlink detection, a lot of programs key things by (dev, ino), and
some inode numbers are guaranteed by the kernel for pseudo-filesystems
like PROC_ROOT_INO).
[1]: https://lwn.net/Articles/975444/
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Florian Weimer @ 2024-07-29 11:36 UTC (permalink / raw)
To: Mateusz Guzik
Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner,
Christian Brauner
In-Reply-To: <CAGudoHEBNRE+78n=WEY=Z0ZCnLmDFadisR-K2ah4SUO6uSm4TA@mail.gmail.com>
* Mateusz Guzik:
> On Mon, Jul 29, 2024 at 12:57 PM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Mateusz Guzik:
>>
>> > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
>> >> * Mateusz Guzik:
>> >>
>> >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
>> >> >> It was pointed out to me that inode numbers on Linux are no longer
>> >> >> expected to be unique per file system, even for local file systems.
>> >> >
>> >> > I don't know if I'm parsing this correctly.
>> >> >
>> >> > Are you claiming on-disk inode numbers are not guaranteed unique per
>> >> > filesystem? It sounds like utter breakage, with capital 'f'.
>> >>
>> >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
>> >> local file systems are different.
>> >
>> > Can you link me some threads about this?
>>
>> Sorry, it was an internal thread. It's supposed to be common knowledge
>> among Linux file system developers. Aleksa referenced LSF/MM
>> discussions.
>>
>
> So much for open development :-P
I found this pretty quickly, so it does seem widely known:
[LSF TOPIC] statx extensions for subvol/snapshot filesystems & more
<https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/>
>> It's certainly much easier to use than name_to_handle_at, so it looks
>> like a useful option to have.
>>
>> Could we return a three-way comparison result for sorting? Or would
>> that expose too much about kernel pointer values?
>>
>
> As is this would sort by inode *address* which I don't believe is of
> any use -- the order has to be assumed arbitrary.
Doesn't the order remain valid while the files remain open? Anything
else doesn't seem reasonable to expect anyway.
Thanks,
Florian
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Mateusz Guzik @ 2024-07-29 11:06 UTC (permalink / raw)
To: Florian Weimer
Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner,
Christian Brauner
In-Reply-To: <87sevspit1.fsf@oldenburg.str.redhat.com>
On Mon, Jul 29, 2024 at 12:57 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Mateusz Guzik:
>
> > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> >> * Mateusz Guzik:
> >>
> >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> >> >> It was pointed out to me that inode numbers on Linux are no longer
> >> >> expected to be unique per file system, even for local file systems.
> >> >
> >> > I don't know if I'm parsing this correctly.
> >> >
> >> > Are you claiming on-disk inode numbers are not guaranteed unique per
> >> > filesystem? It sounds like utter breakage, with capital 'f'.
> >>
> >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> >> local file systems are different.
> >
> > Can you link me some threads about this?
>
> Sorry, it was an internal thread. It's supposed to be common knowledge
> among Linux file system developers. Aleksa referenced LSF/MM
> discussions.
>
So much for open development :-P
> > I had this in mind (untested modulo compilation):
> >
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index 300e5d9ad913..5723c3e82eac 100644
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -343,6 +343,13 @@ static long f_dupfd_query(int fd, struct file *filp)
> > return f.file == filp;
> > }
> >
> > +static long f_dupfd_query_inode(int fd, struct file *filp)
> > +{
> > + CLASS(fd_raw, f)(fd);
> > +
> > + return f.file->f_inode == filp->f_inode;
> > +}
> > +
> > static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > struct file *filp)
> > {
> > @@ -361,6 +368,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > case F_DUPFD_QUERY:
> > err = f_dupfd_query(argi, filp);
> > break;
> > + case F_DUPFD_QUERY_INODE:
> > + err = f_dupfd_query_inode(argi, filp);
> > + break;
> > case F_GETFD:
> > err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
> > break;
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index c0bcc185fa48..2e93dbdd8fd2 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -16,6 +16,8 @@
> >
> > #define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
> >
> > +#define F_DUPFD_QUERY_INODE (F_LINUX_SPECIFIC_BASE + 4)
> > +
> > /*
> > * Cancel a blocking posix lock; internal use only until we expose an
> > * asynchronous lock api to userspace:
>
> It's certainly much easier to use than name_to_handle_at, so it looks
> like a useful option to have.
>
> Could we return a three-way comparison result for sorting? Or would
> that expose too much about kernel pointer values?
>
As is this would sort by inode *address* which I don't believe is of
any use -- the order has to be assumed arbitrary.
Perhaps there is something which is reliably the same and can be
combined with something else to be unique system-wide (the magic
handle thing?).
But even then you would need to justify trying to sort by fcntl calls,
which sounds pretty dodgey to me.
Given that thing I *suspect* statx() may want to get extended with
some guaranteed unique identifier. Then you can sort in userspace all
you want.
Based on your opening mail I assumed you only need to check 2 files,
for which the proposed fcntl does the trick.
Or to put it differently: there seems to be more to the picture than
in the opening mail, so perhaps you could outline what you are looking
for.
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Florian Weimer @ 2024-07-29 10:57 UTC (permalink / raw)
To: Mateusz Guzik; +Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner
In-Reply-To: <vmjtzzz7sxctmf7qrf6mw5hdd653elsi423joiiusahei22bft@quvxy4kajtxt>
* Mateusz Guzik:
> On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
>> * Mateusz Guzik:
>>
>> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
>> >> It was pointed out to me that inode numbers on Linux are no longer
>> >> expected to be unique per file system, even for local file systems.
>> >
>> > I don't know if I'm parsing this correctly.
>> >
>> > Are you claiming on-disk inode numbers are not guaranteed unique per
>> > filesystem? It sounds like utter breakage, with capital 'f'.
>>
>> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
>> local file systems are different.
>
> Can you link me some threads about this?
Sorry, it was an internal thread. It's supposed to be common knowledge
among Linux file system developers. Aleksa referenced LSF/MM
discussions.
> I had this in mind (untested modulo compilation):
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 300e5d9ad913..5723c3e82eac 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -343,6 +343,13 @@ static long f_dupfd_query(int fd, struct file *filp)
> return f.file == filp;
> }
>
> +static long f_dupfd_query_inode(int fd, struct file *filp)
> +{
> + CLASS(fd_raw, f)(fd);
> +
> + return f.file->f_inode == filp->f_inode;
> +}
> +
> static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> struct file *filp)
> {
> @@ -361,6 +368,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> case F_DUPFD_QUERY:
> err = f_dupfd_query(argi, filp);
> break;
> + case F_DUPFD_QUERY_INODE:
> + err = f_dupfd_query_inode(argi, filp);
> + break;
> case F_GETFD:
> err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
> break;
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index c0bcc185fa48..2e93dbdd8fd2 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -16,6 +16,8 @@
>
> #define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
>
> +#define F_DUPFD_QUERY_INODE (F_LINUX_SPECIFIC_BASE + 4)
> +
> /*
> * Cancel a blocking posix lock; internal use only until we expose an
> * asynchronous lock api to userspace:
It's certainly much easier to use than name_to_handle_at, so it looks
like a useful option to have.
Could we return a three-way comparison result for sorting? Or would
that expose too much about kernel pointer values?
Thanks,
Florian
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Mateusz Guzik @ 2024-07-29 10:56 UTC (permalink / raw)
To: Florian Weimer; +Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner
In-Reply-To: <vmjtzzz7sxctmf7qrf6mw5hdd653elsi423joiiusahei22bft@quvxy4kajtxt>
On Mon, Jul 29, 2024 at 12:50 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> > * Mateusz Guzik:
> >
> > > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> > >> It was pointed out to me that inode numbers on Linux are no longer
> > >> expected to be unique per file system, even for local file systems.
> > >
> > > I don't know if I'm parsing this correctly.
> > >
> > > Are you claiming on-disk inode numbers are not guaranteed unique per
> > > filesystem? It sounds like utter breakage, with capital 'f'.
> >
> > Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> > local file systems are different.
> >
>
> Can you link me some threads about this?
>
> > > While the above is not what's needed here, I guess it sets a precedent
> > > for F_DUPINODE_QUERY (or whatever other name) to be added to handily
> > > compare inode pointers. It may be worthwhile regardless of the above.
> > > (or maybe kcmp could be extended?)
> >
> > I looked at kcmp as well, but I think it's dependent on
> > checkpoint/restore. File sameness checks are much more basic than that.
> >
>
> I had this in mind (untested modulo compilation):
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 300e5d9ad913..5723c3e82eac 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -343,6 +343,13 @@ static long f_dupfd_query(int fd, struct file *filp)
> return f.file == filp;
> }
>
> +static long f_dupfd_query_inode(int fd, struct file *filp)
> +{
> + CLASS(fd_raw, f)(fd);
> +
> + return f.file->f_inode == filp->f_inode;
> +}
> +
> static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> struct file *filp)
> {
> @@ -361,6 +368,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> case F_DUPFD_QUERY:
> err = f_dupfd_query(argi, filp);
> break;
> + case F_DUPFD_QUERY_INODE:
> + err = f_dupfd_query_inode(argi, filp);
> + break;
> case F_GETFD:
> err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
> break;
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index c0bcc185fa48..2e93dbdd8fd2 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -16,6 +16,8 @@
>
> #define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
>
> +#define F_DUPFD_QUERY_INODE (F_LINUX_SPECIFIC_BASE + 4)
> +
> /*
> * Cancel a blocking posix lock; internal use only until we expose an
> * asynchronous lock api to userspace:
To clarify, if indeed the dev + ino combo is no longer sufficient to
conclude it's the same thing, an explicit & handy to use way should be
provided.
For a case where you got both inodes open something like the above
will do the trick, unless I'm missing something. I can do a proper
patch posting later after runtime testing and whatnot if you confirm
this sorts out your problem.
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply
* Re: Testing if two open descriptors refer to the same inode
From: Mateusz Guzik @ 2024-07-29 10:50 UTC (permalink / raw)
To: Florian Weimer; +Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner
In-Reply-To: <875xsoqy58.fsf@oldenburg.str.redhat.com>
On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> * Mateusz Guzik:
>
> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> >> It was pointed out to me that inode numbers on Linux are no longer
> >> expected to be unique per file system, even for local file systems.
> >
> > I don't know if I'm parsing this correctly.
> >
> > Are you claiming on-disk inode numbers are not guaranteed unique per
> > filesystem? It sounds like utter breakage, with capital 'f'.
>
> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> local file systems are different.
>
Can you link me some threads about this?
> > While the above is not what's needed here, I guess it sets a precedent
> > for F_DUPINODE_QUERY (or whatever other name) to be added to handily
> > compare inode pointers. It may be worthwhile regardless of the above.
> > (or maybe kcmp could be extended?)
>
> I looked at kcmp as well, but I think it's dependent on
> checkpoint/restore. File sameness checks are much more basic than that.
>
I had this in mind (untested modulo compilation):
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 300e5d9ad913..5723c3e82eac 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -343,6 +343,13 @@ static long f_dupfd_query(int fd, struct file *filp)
return f.file == filp;
}
+static long f_dupfd_query_inode(int fd, struct file *filp)
+{
+ CLASS(fd_raw, f)(fd);
+
+ return f.file->f_inode == filp->f_inode;
+}
+
static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
struct file *filp)
{
@@ -361,6 +368,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
case F_DUPFD_QUERY:
err = f_dupfd_query(argi, filp);
break;
+ case F_DUPFD_QUERY_INODE:
+ err = f_dupfd_query_inode(argi, filp);
+ break;
case F_GETFD:
err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
break;
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index c0bcc185fa48..2e93dbdd8fd2 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -16,6 +16,8 @@
#define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
+#define F_DUPFD_QUERY_INODE (F_LINUX_SPECIFIC_BASE + 4)
+
/*
* Cancel a blocking posix lock; internal use only until we expose an
* asynchronous lock api to userspace:
^ permalink raw reply related
* Re: Testing if two open descriptors refer to the same inode
From: Florian Weimer @ 2024-07-29 10:40 UTC (permalink / raw)
To: Mateusz Guzik; +Cc: linux-fsdevel, linux-kernel, linux-api, Dave Chinner
In-Reply-To: <ghqndyn4x7ujxvybbwet5vxiahus4zey6nkfsv6he3d4en6ehu@bq5s23lstzor>
* Mateusz Guzik:
> On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
>> It was pointed out to me that inode numbers on Linux are no longer
>> expected to be unique per file system, even for local file systems.
>
> I don't know if I'm parsing this correctly.
>
> Are you claiming on-disk inode numbers are not guaranteed unique per
> filesystem? It sounds like utter breakage, with capital 'f'.
Yes, POSIX semantics and traditional Linux semantics for POSIX-like
local file systems are different.
> While the above is not what's needed here, I guess it sets a precedent
> for F_DUPINODE_QUERY (or whatever other name) to be added to handily
> compare inode pointers. It may be worthwhile regardless of the above.
> (or maybe kcmp could be extended?)
I looked at kcmp as well, but I think it's dependent on
checkpoint/restore. File sameness checks are much more basic than that.
Thanks,
Florian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox