Bug 5342

Summary: Продвижение cifs-патчей в upstream
Product: CIFS@Etersoft Reporter: Pavel Shilovsky <piastry>
Component: ДолжностныеAssignee: Pavel Shilovsky <piastry>
Status: CLOSED FIXED QA Contact: Vitaly Lipatov <lav>
Severity: normal    
Priority: P4 CC: lav, sin
Version: не указана   
Target Milestone: ---   
Hardware: PC   
OS: All   
Whiteboard:
Заявки RT: Связано с:
Дата напоминания: 1th

Description Pavel Shilovsky 2010-03-24 13:31:13 MSK
Так как патчей накопилось уже достаточно много, то есть смысл попробовать прокинуть часть из них, которые не касаются архитектурных особенностей wine в ядро. При удачном раскладе, будет проще добавлять новые ядра - только применяя к ним патчи для wine.
Comment 1 Pavel Shilovsky 2010-03-24 13:51:49 MSK
На данный момент начал продвижение следующих патчей:
1. mandatory reading (ветка etersoft-mandatory-reading).
2. getflk problem (etersoft-getflk).
3. direct problem (etersoft-direct) - для direct+nobrl (direct, без nobrl уже в upstream).
Comment 2 Pavel Shilovsky 2010-03-25 10:46:17 MSK
Патчи рассматриваются, ведётся обсуждение.
Comment 3 Pavel Shilovsky 2010-03-25 10:46:53 MSK
http://patchwork.ozlabs.org/project/linux-cifs-client/list/
обсуждение можно посмотреть здесь.
Comment 4 Pavel Shilovsky 2010-03-28 22:45:52 MSD
Патч "direct problem" принят: коммит 810627a013163cd294762d57c0ea2ec055ffe4f6.
По двум другим патчам ведётся обсуждение, причём патч "getlk problem" уже готовы принять, - переоформил их согласно требованиям (стиль и подробное описание) и выслал заново.
Comment 5 Pavel Shilovsky 2010-04-06 09:34:03 MSD
Отправил ещё один патч, связанный с потерей блокировки в главном процессе после завершения порождённого процесса - блокировку нельзя снять, но она есть.

По предыдущим патчам: патч 1 вызвал дискуссию о целесообразности использования данного подхода вместо простого включения опции direct - буду пытаться тестами выяснить улучшится ли производительность; патч 2 принимают, но пока его нет в upstream.
Comment 6 Pavel Shilovsky 2010-04-06 22:48:47 MSD
Патч getlk в upstream - коммит f05337c6ac48d19d354e0640a8eb8fc884f82bcc.
Comment 7 Pavel Shilovsky 2010-04-22 09:40:12 MSD
Патч, связанный с потерей блокировки принят в upstream:
коммит 2c964d1f7c87eb71f7902111cd7c8fbba225e4b6.
Comment 8 Pavel Shilovsky 2010-09-08 11:02:51 MSD
Поднял тему обсуждения проблемы кеша в CIFS в рассылке разработчиков. По результатам тестирования в баге 5442 можно сделать вывод, что код, который следует спецификации (сборка 4.5.1.4) не ломает локику работы приложений, но использует кеш при одиночном доступе к файлу.
Comment 9 Pavel Shilovsky 2010-09-09 22:35:05 MSD
Обсуждение начато, можно наблюдать на http://news.gmane.org/gmane.linux.kernel.cifs - тред CIFS data coherency problem.
Comment 10 Pavel Shilovsky 2010-09-10 11:55:14 MSD
Отправил ещё один тест, подтверждающий проблему. Так же описал проблему с одной шарой, подмонтированной в несколько директорий - Oplock break возвращается сервером только в одну.
Comment 11 Pavel Shilovsky 2010-09-11 00:30:14 MSD
Заметил, что результаты работы с оплоками отличаются для Samba 4.0.0-alpha11 и Samba-3.4.7 (Ubuntu).

Samba-4:
1) client1 открыл файл с oplock ex;
2) client1 записал в файл4
3) client2 открыл файл с oplock level II, на client1 oplock стал тоже level II;
4) client2 записал в файл и получил oplock level None;
5) client1 открыл файл через другой дескриптор, получил oplock level II.

Samba-3.4.7:
1) client1 открыл файл с oplock ex;
2) client1 записал в файл:
3) client2 открыл файл с oplock None, на client1 oplock стал тоже None;
4) client2 записал в файл;
5) client1 открыл файл через другой дескриптор, получил oplock None.

Во втором случае поведение корректное, в первом нет, так как client1 не получает  Oplock break to None после того, как client2 записал в файл.
Comment 12 Pavel Shilovsky 2010-09-11 11:12:04 MSD
Спецификация MS-CIFS не содержит точного упоминания по поводу установки поля NewOplockLevel в Oplock Break Acknowledge. Сделал вывод, что оно просто не используется, продолжаем обсуждать с разработчиками проблему несбрасывания сервером Samba 4.0.0-alpha11 оплоков level II для всех открытых дескрипторов.
Comment 13 Pavel Shilovsky 2010-09-11 14:48:24 MSD
Подготовил и отправил патч по баги 5442 в апстрим. Ждём результатов.
Comment 14 Pavel Shilovsky 2010-09-13 16:04:32 MSD
После обсуждений пришли к выводу, что надо разбить патч на три независимых патча:
1) пропуск при чтении кеша если мы не имеем оплока на запись или чтение;
2) установка кеша в неактуальное состояние после закрытия последнего дескриптора;
3) запись на сразу сервер если у нас нет оплока на запись.

Последний патч вызвал замечания по поводу производительности, собираюсь поправить.
Comment 15 Pavel Shilovsky 2010-09-18 10:48:52 MSD
Отправил первый и второй патч в upstream. Начал работу над третьим.
Comment 16 Pavel Shilovsky 2010-09-27 22:35:28 MSD
По первым двум патчам завязалось обсуждение, в процессе которого выяснилось, что стоит добавить новую опцию монтирования для данного поведения. Добавил опцию strictcache, поправил патчи (1-ый и 2-ой) и отправил заново.
Comment 17 Pavel Shilovsky 2010-11-03 11:05:37 MSK
Поправил патч из http://bugs.etersoft.ru/show_bug.cgi?id=6227#c9, отправил в рассылку.
Comment 18 Pavel Shilovsky 2010-11-05 20:51:17 MSK
Патч из http://bugs.etersoft.ru/show_bug.cgi?id=5342#c17 принят в upstream.
Comment 19 Pavel Shilovsky 2010-11-09 19:53:27 MSK
Поправил патч из баги 4875 и создал патч для upstream. Отправил.
Comment 20 Pavel Shilovsky 2010-11-14 14:36:21 MSK
Во время обсуждения патча из баги #4875, решено было переписать код, устанавливающий соединения с сокетом в cifs. Сделал это и отправил патчи на обсуждение.
1) объединил функции ipv4_connect и ipv6_connect в одну, убрав много дублирующего кода. Отделил код, работающий с RFC1001 серверами (порт 139).
2) добавил отдельную функцию match_port, в которую прокинул наши исправления из баги #4875.
Comment 21 Pavel Shilovsky 2010-11-14 21:52:10 MSK
Поправил патчи, что отправил утром. Переотправил.
Comment 22 Pavel Shilovsky 2010-11-15 09:14:21 MSK
Патчи вызвали дискуссию, я попытался подробнее объяснить проблему текущего кода. Вот письмо, что я составил и отправил:

"Let's predict the following situation:

we have server with two smbd's: 1th on 445 port, and 2nd on 5000 port.
At first user mounts 2nd smbd with specifying 5000 port. Then it tries
to mount 1th without specifying any ports. If we consider your logic,
our client looks for an any existing connection, finds connection to
5000 port (2nd smbd) and mounts it. So, as the result, user thinks
that it successfully mounted the 1th share, but it is the 2nd - it is
not right.

If we should follow mount.cifs spec, I think we need to do:
1) if user doesn't specify a port we at first try to match a
connection with 445 port, then if we are not successful - with 139
port. If there are no connections with these ports, we return an
error.
2) if user specifies a port, we should try to match a connection only
with this port. If no connections with this port, we should return an
error.

I think that it is real problem. I faced with a real application
freenx server that uses CIFS for mounting many shares on different
ports (freenx clients want mount to mount its own share through ssh
port forwarding - many smbds with non-standard ports and local smbd
with standard port) on localhost.

You can try the following to reproduce the bug:
run smbd's on localhost and smbserver_host (another).

On the first console:
ssh -L5000:smbserver_host:445 smbserver_host
On the second:
mount -t cifs //localhost/share ~/test -oport=5000
mount -t cifs //localhost/share ~/test2

As the result, we can see that the second call connects to
smbserve_host rather than to localhost and return without any error -
so, user doesn't know that it has mounted smbserver_host instead of
localhost.

Even more, if we unmounted and mount them vice versa:
mount -t cifs //localhost/share ~/test2
mount -t cifs //localhost/share ~/test -oport=5000

it will be ok - two different connection to two different share, but
(!) if we losing the connection to port 5000, today's code reconnects
with share to localhost - wrong again! My patch number 1 fixes this
situation too.

I am ready to provide changes to the patches I posted later according
to mount.cifs spec as soon as we find an agreement in all the things"
Comment 23 Pavel Shilovsky 2010-11-15 16:34:15 MSK
Получил комментарии по первому патчу (который делает коде рефакторинг в организации установки соединения с сервером). Собираюсь сделать следующее:
1) сделать код функции ip_connect независимым от порта и перенести логику выбора порта для соединения в другую функцию;
2) исправить некоторые баги и неточности в данном патчи.
Comment 24 Pavel Shilovsky 2010-11-16 11:05:25 MSK
Поправил оба патча - теперь логика следующая:
1) если у мы указываем порт, то используется только он.
2) если мы не указываем порт, то сначала пробуется 445 порт и если не удаётся - 139 порт.

Так же при поиске существующего соединения мы:
1) если указан порт, то ищём с ним.
2) если не указан, то ищем либо 445 либо 139 порт.

Отправил в рассылку. В случае успеха, дальше прокину эту логику в наши ветки.
(сейчас, начиная с 4.5.5, наш код отличается тем, что при поиске существующего соединения если порт не указан, мы ищем только 445 порт). Таким образом в случае 139 порта мы имеем на каждое соединения отдельный сокет.
Comment 25 Pavel Shilovsky 2010-11-18 10:51:31 MSK
Сделал патч для mount.cifs man страницы, изменив описание логики работы опции 'port=', согласно описанному выше.
Comment 26 Pavel Shilovsky 2010-11-25 13:45:43 MSK
Отныне тут буду только регистрировать результат продвижения патчей. Процесс их разработки отмечается в баге http://bugs.etersoft.ru/show_bug.cgi?id=6517.
Comment 27 Pavel Shilovsky 2010-12-13 19:40:55 MSK
Выяснилось, что патчи связанные с ip_connect потерялись как-то. Переотправил.
Comment 28 Pavel Shilovsky 2010-12-16 19:51:19 MSK
Обсудил текущие патчи прошедшие ревью (helper, port) - на днях ожидаю их появление в апстрим. Также выяснилась проблема в патче writepages из группы патчей описанно тут (http://bugs.etersoft.ru/show_bug.cgi?id=6633#c1).
Comment 30 Pavel Shilovsky 2010-12-29 11:52:36 MSK
Переоформил патчи группы helper с полученными reviewed-by тэгами и отправил Стиву Френчу.
Comment 32 Pavel Shilovsky 2011-01-15 09:53:01 MSK
Обновил master на git.eter - патчи группы helper и port в главной ветке ядра.
Comment 33 Pavel Shilovsky 2011-04-21 22:29:41 MSK
После личной беседы со Стивом Френчем патч, исправляющий разбор опций монтирования, включён в master на git.kernel.org. Так же обсудили патчи, решающие проблему обработки ошибок к логиче валидации айнода - первый патч принят (из http://bugs.etersoft.ru/show_bug.cgi?id=6940#c24), второй требует большего осмысления и доработки.
Comment 34 Pavel Shilovsky 2011-05-11 16:11:11 MSK
Отправил патч, исправляющий ошибку в разборе опций монтирования d stable ветку (http://bugs.etersoft.ru/show_bug.cgi?id=7055#c15).
Comment 35 Pavel Shilovsky 2011-05-22 13:26:36 MSK
Патч из http://bugs.etersoft.ru/show_bug.cgi?id=6517#c44 в основной ветке ядра.
Comment 37 Pavel Shilovsky 2011-05-27 12:14:23 MSK
Отослал в рассылку патч для man mount.cifs, исправлющий описание опции wsize согласно новому режиму разделённого суперблока.
Comment 38 Pavel Shilovsky 2011-05-27 12:28:32 MSK
Отослал в рассылку патч для man mount.cifs, добавляющий информацию об опции монтирования rwpidforward.
Comment 39 Pavel Shilovsky 2011-05-28 10:31:45 MSK
Патчи режима разделённого суперблока и проброса пида в основной ветке ядра: http://git.kernel.org/?p=linux/kernel/git/sfrench/cifs-2.6.git;a=commit;h=40efeb4d0bb1993c3c10baff9b7d86839f99171e
Comment 40 Pavel Shilovsky 2011-06-01 23:20:42 MSK
Патчи из комментариев 37 и 38 в апстрим.
Comment 41 Pavel Shilovsky 2011-10-25 14:16:45 MSK
Бага себя исчерпала. Дальнейшее продвижение патчей будет регистрироваться в одноимённых багах по их разработке.