Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Filelock #11724

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Implement Filelock #11724

merged 8 commits into from
Feb 21, 2024

Conversation

crafcat7
Copy link
Contributor

Summary

I made functional implementations of F_SETLK, F_SETLKW, F_GETLK in fcntl to extend the capabilities of fcntl.
The way to implement this is to use a simple Hashtable implementation, using the path as the key to store the lock information context of a file, from which you can go to the lock, unlock, merge locks and other operations.

Impact

Implemented file locking functionality, extending the F_SETLK, F_SETLKW, F_GETLK implementations in fcntl.

Testing

Through a series of tests on file locks, its functionality aligns with Linux file locks

guohao15 and others added 6 commits February 20, 2024 16:11
merge form OpenGroup
https://pubs.opengroup.org/onlinepubs/009696899/functions/hcreate.html

Signed-off-by: guohao15 <guohao15@xiaomi.com>
Signed-off-by: guohao15 <guohao15@xiaomi.com>
Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
When there is no conflicting lock, getlk should not tamper with the l_pid content from the upper layer

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
When we close a socket fd, it will call get path on sockets.
`close(socket_fd)` -> `file_closelk(filep)` -> `file_fcntl(F_GETPATH)`
It causes a heavy stack load for each socket close operation.
(We have `GETPATH` for sockets to be used for `fdinfo`)

But the socket fds are not intended to be used for file locks.
And so do some other file types, so we may just limit the usage of flock.

Signed-off-by: Zhe Weng <wengzhe@xiaomi.com>
@jturnsek
Copy link
Contributor

If underlying fs is on a MTD device, function call file_fcntl(filep, F_GETPATH, path) returns with ENOTTY in my case where MX25L device is used to serve littlefs fileystem.

@xiaoxiang781216
Copy link
Contributor

If underlying fs is on a MTD device, function call file_fcntl(filep, F_GETPATH, path) returns with ENOTTY in my case where MX25L device is used to serve littlefs fileystem.

our littlefs fs wrapper implement F_GETPATH to return the file name

@crafcat7
Copy link
Contributor Author

If underlying fs is on a MTD device, function call file_fcntl(filep, F_GETPATH, path) returns with ENOTTY in my case where MX25L device is used to serve littlefs fileystem.

Please try this change #11729. I will submit the LFS support GET PATH implementation to another PR.

fs/vfs/fs_lock.c Outdated Show resolved Hide resolved
Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
@acassis acassis merged commit ae730ac into apache:master Feb 21, 2024
27 checks passed
@yamt
Copy link
Contributor

yamt commented May 13, 2024

using a path as a file id for locking seems broken by design. (consider rename.)
can you consider to use something more appropriate?
eg. a dedicated id for file locking, which is not changed by a rename.

@xiaoxiang781216
Copy link
Contributor

but how to get id from the different file system?

@yamt
Copy link
Contributor

yamt commented May 14, 2024

add it to file_operations. or it can be fcntl/ioctl as F_GETPATH.

@xiaoxiang781216
Copy link
Contributor

The problem is that many fs designed for RTOS doesn't define the unique id in file metadata, adding file_operations can't resolve this problem.

@xiaoxiang781216
Copy link
Contributor

I don't object to utilize some unique id, but this approach need improve all file system implementation, which isn't a minor work. The current implementation of filelock work well in most case. If your case require handle rename correctly, feel free to improve the current implementation, thanks.

@yamt
Copy link
Contributor

yamt commented May 14, 2024

well, actually, i don't have a motivation to improve file locking.
i'm just unhappy with having the local littlefs patch.

@xiaoxiang781216
Copy link
Contributor

If you don't use file lock, could skip the local patch safely.

@yamt
Copy link
Contributor

yamt commented May 15, 2024

If you don't use file lock, could skip the local patch safely.

it doesn't solve the maintenance problem.

if no one wants to work on a solution, i'd suggest to revert it.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented May 15, 2024

If you don't use file lock, could skip the local patch safely.

it doesn't solve the maintenance problem.

if no one wants to work on a solution, i'd suggest to revert it.

This feature is used by other community member not us. It work fine, why do you want to revert it?

@yamt
Copy link
Contributor

yamt commented May 17, 2024

If you don't use file lock, could skip the local patch safely.

it doesn't solve the maintenance problem.
if no one wants to work on a solution, i'd suggest to revert it.

This feature is used by other community member not us. It work fine, why do you want to revert it?

because it's responsibility for the person adding a feature to find a maintainable implementation.

my suggestion is

  • introduce an optional api to query filesystems file id usable for this purpose. say, F_GETIDFORLOCK
  • adapt this PR to use F_GETIDFORLOCK. when a filesystem doesn't support it, probably fall back to F_GETPATH. or just bail out with ENOTSUP or such.
  • revert F_GETPATH patch in littlefs because it seems unacceptable for the upstream.
  • add necessary api to the upstream littlefs, probably something along the line with: Support get file path littlefs-project/littlefs#976 (comment). importantly we should provide something the upstream littlefs can accept.
  • implement F_GETIDFORLOCK in our littlefs driver, based on the new api. i guess you can iterate all open files and find aliases, and assign/reuse the in-core file id accordingly.

alternatively, you can probably try to convince the littlefs author to accept the get file path patch. if it gets merged in the upstream, i have no big objection against having the patch in our tree for older littlefs versions in the meantime. i personally agree with his rationale of the rejection though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants